Consolidate waking up code #40069
Merged
Consolidate waking up code #40069
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Almost the same code was present in avatar::wake_up and player_hardcoded_effects. Consolidate the two into one implementation.
ifreund
reviewed
May 2, 2020
Comment on lines
-1254
to
-1255
| get_effect( effect_slept_through_alarm ).set_duration( 0_turns ); | ||
| remove_effect( effect_alarm_clock ); |
There was a problem hiding this comment.
I don't see these two lines present after the change. Is that intentional?
There was a problem hiding this comment.
avatar::wake_up calls Character::wake_up, and that removes these two effects (albeit with slightly different code).
ifreund
added
[C++]
This was referenced May 3, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
SUMMARY: None
Purpose of change
Almost the same code was present in
avatar::wake_upandplayer_hardcoded_effects. In particular, this code duplication meant that I missed some wake up cases when I added the event for waking up, making the survival achievement fail to trigger sometimes.Describe the solution
Consolidate the two into one implementation.
Describe alternatives you've considered
The two were very slightly different. Based on my analysis of the code I think my merged implementation is equivalent or better, but it's hard to think about (or test) every corner case. Waking up interacts with alarms in a complicated way.
Testing
I played the game and ensured that waking normally and via an alarm seemed to work as expected.
Additional context
Discovered while testing achievements.