Skip to content

core: Refresh frame script on registration #20293

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

nivkner
Copy link
Contributor

@nivkner nivkner commented Apr 28, 2025

fixes #11467 and fixes #19692

while running a goto on a frame would normally cause its frame-script to be counted as already executed,
a newly registered frame-script does not seem to count as such.

the None case of queued_script_frame is, as far as i can tell, redundant given that last_queued_script_frame already prevents the same script from running twice, so it was removed to allow for simpler "refresh" logic.

@torokati44 torokati44 added A-core Area: Core player, where no other category fits T-fix Type: Bug fix (in something that's supposed to work already) labels Apr 28, 2025
@nivkner
Copy link
Contributor Author

nivkner commented Apr 29, 2025

to double-check the point about None (and for the fun of it),
I wrote a TLA+ specification to verify that the Option is in fact redundant here.

what i got was that it is redundant as of 64d6bbf, so long as run_frame_internal is called at least once before run_frame_scripts.

given that run_frame_scripts is called only during the frame script phase, while run_frame_scripts is called during the enter phase, it seems likely.

EDIT: the explanation is that the only place that sets queued_script_frame to None is the same one that sets last_queued_script_frame to the its current value, so they are the same (and thus the is_fresh check fails, where it would have failed from the let Some(..) = ... check)

@nivkner
Copy link
Contributor Author

nivkner commented May 17, 2025

decided to remove changes to queued_script_frame, to hopefully make it easier to review.

Copy link

@Molisson Molisson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed carefully (test files ignored) and seemed simple for me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-core Area: Core player, where no other category fits newsworthy T-fix Type: Bug fix (in something that's supposed to work already)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Snail Bob 2: First level softlock Snail Bob: softlock, can not click on next level button
4 participants