chore: Prettifies get_ending to not be numerically based, but instead string-based. #219

san7890 · 2023-05-06T06:36:02Z

san7890 commented

2023-05-06 06:36:02 +00:00

Hey there,

I'm not sure if this project accepts unsolicited contributions (without an issue), but I played through the game and really enjoyed it. While doing so, I noticed some patterns that really made me :(. Instead of feeling :(, I decided to clean it up to pay it forward to whoever wants to extend this really neat game.

This PR does the following:

get_ending method now returns SCREAMING_SNAKE_CASE variables that resolve to strings. This is mainly done as a "code improvement", such that the code is no longer reliant on the "funny numbers" 1, 2, 3, and 4 and not needing to rely on a comment to explain what 1, 2, 3, and 4 mean.
- This should probably help with modding? I don't think anyone is going to put the time into making a brand new ending for official implementation any time soon. At the very least, I believe it is more readability and less prone on the explanatory comment - it relies on itself.
The code has also been amended to reflect this change (i really wish renpy had switch 🙁)
- These were all pretty trivial (as well as tested to the best of my abilities), except for one of the final path commits. I ran into an issue where it would spit me back to Chapter 2, but that might be the result of my own misdoing with modifying the game carrying over? I didn't bother to clean my dev environment to check out (let me know if I should) but I decided to change it to a much more explicit early-return function to change the story to chapter_11A (849c9ce9f5), which is also good because the indentation looks slightly nicer to my eyes. I'm not new to python, but I am new to the unique synax of RenPy so let me know if dcb7fbb473 is un-needed or if the jump function does that for us.
Cleans up credit code as well.
- We were doing the same exact calculations that we were doing in get_ending for some reason, despite the fact that we weren't actually calling get_ending. That seems very silly to me, so I decided to tie a method-local var (are they methods?) that assumes the return value from get_ending and we just use that to do all our credit determinations rather than rely on calling the tradwife flag or doing the same exact calculations but different. Should help with less "drifting from reality" between the different credit sequences and the actual requirements for those credits.

I spent more time writing about what I did than actually doing the code (testing took a while longer), so hopefully it is no-more-than-trivial to review (and hopefully doesn't break stuff (but that's why Git is so good: reverts)). Thank you for your time.

note: This PR is also targetting the 'NewPatch' since the readme said to not push to master? I assume that's the best spot, please let me know if I should change it.

Hey there, I'm not sure if this project accepts unsolicited contributions (without an issue), but I played through the game and really enjoyed it. While doing so, I noticed some patterns that really made me :(. Instead of feeling :(, I decided to clean it up to pay it forward to whoever wants to extend this really neat game. This PR does the following: * `get_ending` method now returns SCREAMING_SNAKE_CASE variables that resolve to strings. This is mainly done as a "code improvement", such that the code is no longer reliant on the "funny numbers" 1, 2, 3, and 4 and not needing to rely on a comment to explain what 1, 2, 3, and 4 mean. * * This should probably help with modding? I don't think anyone is going to put the time into making a brand new ending for official implementation any time soon. At the very least, I believe it is more readability and less prone on the explanatory comment - it relies on itself. * The code has also been amended to reflect this change (i really wish renpy had switch 🙁) * * These were all pretty trivial (as well as tested to the best of my abilities), except for one of the final path commits. I ran into an issue where it would spit me back to Chapter 2, but that might be the result of my own misdoing with modifying the game carrying over? I didn't bother to clean my dev environment to check out (let me know if I should) but I decided to change it to a much more explicit early-return function to change the story to chapter_11A (849c9ce9f53a505e8f065c3784b8f7491e34c8c2), which is also good because the indentation looks slightly nicer to my eyes. I'm not new to python, but I am new to the unique synax of RenPy so let me know if dcb7fbb47383026163eaf6ca526b8f2f9e379626 is un-needed or if the `jump` function does that for us. * Cleans up credit code as well. * * We were doing the same exact calculations that we were doing in `get_ending` for some reason, despite the fact that we weren't actually calling `get_ending`. That seems very silly to me, so I decided to tie a method-local var (are they methods?) that assumes the return value from `get_ending` and we just use that to do all our credit determinations rather than rely on calling the tradwife flag or doing the same exact calculations but different. Should help with less "drifting from reality" between the different credit sequences and the actual requirements for those credits. I spent more time writing about what I did than actually doing the code (testing took a while longer), so hopefully it is no-more-than-trivial to review (and hopefully doesn't break stuff (but that's why Git is so good: reverts)). Thank you for your time. note: This PR is also targetting the 'NewPatch' since the readme said to not push to master? I assume that's the best spot, please let me know if I should change it.

san7890 added 4 commits 2023-05-06 06:36:19 +00:00

81c07e3017 first implementation

849c9ce9f5 chapter_11A broke for some reason so let's fix that

dcb7fbb473 i think i'm being a paranoid fuck here so i'm doing it as it's own commit

ez pz revert ftw

9f7377a3eb cleans up credits code

tested™️

MichaelYick commented

2023-05-06 06:56:17 +00:00

Thanks for the contribution, we do accept random contributions on our projects (although there is no guarantee they'll be merged).

The changes seem fine, but I think using an enum would be superior to flat string comparison given that string comparisons are much more expensive than simple integer checking. I'll submit a proper review here in a moment.

Thanks for the contribution, we do accept random contributions on our projects (although there is no guarantee they'll be merged). The changes seem fine, but I think using an [enum](https://docs.python.org/3/library/enum.html) would be superior to flat string comparison given that string comparisons are much more expensive than simple integer checking. I'll submit a proper review here in a moment.

👍 1

MichaelYick requested changes 2023-05-06 06:58:48 +00:00

game/utility.rpy Outdated

						
				@ -3,0 +5,4 @@

				    ENDING_GOLDEN = "golden"

				    ENDING_TRADWIFE = "tradwife"

				    ENDING_DOOMER = "doomer"

				    ENDING_SHOOTER = "shooter"

MichaelYick commented

2023-05-06 06:58:17 +00:00

I heavily suggest swapping these out for enums instead of strings. It has both the user readability of a string, and the simplicity of an integer.

Though, do check if this screws with the ending checking after this is done. I don't know if enums can be compared in python or not.

I heavily suggest swapping these out for enums instead of strings. It has both the user readability of a string, and the simplicity of an integer. Though, do check if this screws with the ending checking after this is done. I don't know if enums can be compared in python or not.

san7890 commented

2023-05-06 21:55:35 +00:00

They can be, implemented in latest commit. Works on my machine.

It's nice that the latest renpy actually supports Python 3.0+ features- enums weren't a thing until 3.4.

They can be, implemented in latest commit. Works on my machine. It's nice that the latest renpy actually supports Python 3.0+ features- enums weren't a thing until 3.4.

MichaelYick marked this conversation as resolved

san7890 added 1 commit 2023-05-06 21:55:15 +00:00

cc1d90d2a9 replace with enums

san7890 added 1 commit 2023-05-06 21:57:53 +00:00

1b5de03b2a tidies up that comment

san7890 requested review from MichaelYick 2023-05-06 21:59:48 +00:00

san7890 added 1 commit 2023-05-06 22:33:07 +00:00

91fcbe3160 whoops that broke

MichaelYick approved these changes 2023-05-07 00:44:05 +00:00

MichaelYick left a comment

Looks good, will merge.

MichaelYick merged commit ef4764b322 into NewPatch