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

core: Global execution order #3104

Closed
wants to merge 25 commits into from
Closed

Conversation

relrelb
Copy link
Contributor

@relrelb relrelb commented Feb 5, 2021

Use a global execution list as suggested in #2943 (comment).

Fixes at least #1626 (now Sonic runs through the corkscrew loop) and #1986 (now enemies get hit).
Doesn't fix other frame-order-execution bugs such as #1164. It might or might not be related.

TODO

  • Add more documentation and comments in the code.
  • Add new tests.

@relrelb relrelb marked this pull request as ready for review February 5, 2021 23:21
@relrelb relrelb marked this pull request as draft February 6, 2021 01:11
@relrelb relrelb force-pushed the global_exec_order branch 2 times, most recently from eb8b348 to 7d316b0 Compare February 6, 2021 17:17
@relrelb relrelb marked this pull request as ready for review February 6, 2021 17:22
@Toad06
Copy link
Member

Toad06 commented Feb 7, 2021

Thanks for your work on this. I've made some testings, using the web extension.

The fixes I've noticed:

There are also unfortunately a few regressions:

@relrelb relrelb marked this pull request as draft February 9, 2021 17:28
@relrelb relrelb force-pushed the global_exec_order branch 3 times, most recently from a3affba to 2008218 Compare February 15, 2021 14:28
@relrelb
Copy link
Contributor Author

relrelb commented Feb 19, 2021

@Toad06 Thanks for the feedback! Your reported regressions should be fixed now. Do you see anything else?

@Toad06
Copy link
Member

Toad06 commented Feb 20, 2021

Thanks, I've made some new testings using the web extension:

  • Steppenwolf 5-1: The character is still "teleported".
  • Zizzo Challenge: The mini-game that was previously fixed no longer works (I've attached an edited version of the game that always plays this mini-game) but maybe this is "expected".
  • Aqua Slug: Seems like there's a performance regression, the game stutters a bit.
  • Jump Girl: Freezes the browser after the level starts.

This is still a work-in-progress prototype.
And also prev_sibling and next_sibling from DisplayObjectBase,
since they are now superseded by the global execution list.
Introduce a struct DepthIter that acts much like RenderIter.
A follow-up refactor needs to remove both DepthIter and RenderIter
in favor of simply using an iterator.
* GlobalExecIter -> ExecIter
* next_global -> next_exec
* prev_global -> prev_exec
This makes 2 regression tests to pass, and doesn't seem to regress
anything else.
As a refactor, remove the now unused bool return value of
remove_from_execution_list.
It might be also worthy for remove_child_from_depth_list
and remove_child_from_render_list, whose bool return values are
unused.
insert_at_id could call to add_to_execution_list while holding a
mutable borrow of self. Then add_to_execution_list mutates the head
of the global execution list, which might be the same object.

Fix it by moving the call to add_to_execution_list out of insert_at_id,
and place it after the call to insert_at_id. add_to_execution_list
should be called conditionally, so also make insert_at_id return a
bool status to indicate whether or not it should be done.
Levels are executed in the following order:
* level0
* The rest of the levels in a stacking creation order (last created level is executed first)
* Correctly insert node to linked list in add_to_execution_list.
* Fix replace_at_depth inserting always to level0.
* Remove redundant scope as a refactoring.
And also add TODOs where iteration order might not be correct.
@relrelb relrelb closed this May 30, 2021
@relrelb relrelb deleted the global_exec_order branch January 18, 2022 20:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants