Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add effect add/remove scheduling #51952

Merged

Conversation

Kribylet
Copy link
Contributor

@Kribylet Kribylet commented Sep 28, 2021

Summary

Bugfixes "Explicitly order effects map changes"

Purpose of change

Fixes #51812. Fixes #51839. Fixes #51900.

Describe the solution

Introduce functions schedule_effect_removal and schedule_effect to the Creature class that can schedule effects to be added/removed after the effects processing step.

Previously effects could be added or removed immediately during the process_effects loop using a "laissez-faire" approach. The iterator might or might not process a newly added effect and effects might be removed before or after being processed. A typical bug I would expect to occur without this PR would be for effect_valium to be applied, create a new effect_downed which would also be processed, then maybe processed again depending on how the iterator would update. Crashes seem to only occur when the current iterator being used is removed due to the effect it is pointing to being removed as part of its own processing.

To the best of my ability to spot the cases, whenever the previous solution tried to immediately add or remove effects the scheduled variants are now used instead. This means that pre-existing effects should always be fully processed before any changes occur to the effects map. The expected order of events is:

  1. Process pre-existing effects
  2. Process immediately added effects (add_effect calls process_one_effect as part of this, part of previous behavior)
  3. Remove effects scheduled for immediate removal
  4. Perform normal effect removal based on duration expiry

To facilitate the scheduling two static std::queue<T> objects were introduced to the Creature class. The queues hold new struct objects that simply store the add_effect and remove_effect function arguments.

After the effects processing loops are completed, the effect additions are performed from the scheduled_effects queue until it is empty. After that, the terminating_effects queue is likewise used to perform any scheduled effect removals. Both queues are emptied by calling the original add_effect and remove_effect functions using the function arguments stored previously.

The schedule_effect functions do not support passing the effect_source as I had some issues with storing the function argument reference during implementation and cut it out during testing. It may be possible to support them, though no current functionality seems to use it.

Reverts #51827.

Describe alternatives you've considered

  1. Restructure the entire effects handling system to inherently support effects that add or remove effects immediately. While the idea has merit, doing so felt vastly out of scope for a bug fix and should probably be the topic of a structured redesign discussion.
  2. Avoid global access type containers for the scheduled effects. While I strongly dislike storing scheduled events as static members of the Creature class as that might create future confusion and inflexibility, I didn't see any other solution that wouldn't require changes to a large amount of functions (reference passing function local containers, changing function signatures) or create needless serialization issues (storing the empty(!) containers in each Creature instance). I might've just missed something obvious here so please let me know if so!
  3. Just scrub out effect removals/additions in the process_effects loop. Replicating the intended behavior through other means would require changes to multiple systems that likely would break a lot of things.

Testing

  1. Tested the crash from Crash without crash report during sleep on Win64 #51812 that stemmed from avatar::wake_up() immediately removing effect_alarm_clock; does not cause a crash in this PR.
  2. Tested Sleeping seems to have been broken into recovering fatigue far slower #51839. Fatigue recovery patterns from sleep matches behavior seen prior to Fix process effects crash #51827.

Testing suggestions:
It is possible that systems that previously "worked" relied on the old behavior and so may behave strangely in this PR.

Additional context

This acts as an alternative solution to the issues resolved by PR #51947.

This is my second PR submitted and while I hope the solution presented is roughly reasonable, I am sure I could benefit from a couple of critical eyes here.

@anothersimulacrum anothersimulacrum added <Bugfix> This is a fix for a bug (or closes open issue) [C++] Changes (can be) made in C++. Previously named `Code` Code: Infrastructure / Style / Static Analysis Code internal infrastructure and style Mechanics: Effects / Skills / Stats Effects / Skills / Stats labels Sep 28, 2021
@RoyBerube
Copy link
Contributor

Nice work. You do need to clean up the code with astyle however - there are over 20 lines that do not comply with the formatting rules.

@Kribylet Kribylet force-pushed the explicitly_order_effects_map_changes branch from f6fe965 to cce8d75 Compare October 4, 2021 15:03
@Kribylet
Copy link
Contributor Author

Kribylet commented Oct 4, 2021

Rebased manually against master to resolve conflicts from #51947 merge.

@ZhilkinSerg ZhilkinSerg merged commit 667fe73 into CleverRaven:master Oct 19, 2021
@Kribylet Kribylet deleted the explicitly_order_effects_map_changes branch October 19, 2021 14:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
<Bugfix> This is a fix for a bug (or closes open issue) [C++] Changes (can be) made in C++. Previously named `Code` Code: Infrastructure / Style / Static Analysis Code internal infrastructure and style Mechanics: Effects / Skills / Stats Effects / Skills / Stats
Projects
None yet
4 participants