Skip to content

core: Run scripts registered at frame script phase in a seperate pass #19941

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 12 commits into
base: master
Choose a base branch
from

Conversation

nivkner
Copy link
Contributor

@nivkner nivkner commented Mar 28, 2025

fixes #10443, and fixes #14629 for the version that does not include buttons.
when buttons are added to the dynamically constructed object, that is a separate issue of the frame script of the child happening before the parent's constructor finishes

im not sure i nailed down all the subtleties regarding the interaction between the cleanup pass and goto,
but the PR passes all the current tests on my machine, and the tests i made,
so i believe it is ready for review as is.

@nivkner nivkner force-pushed the framescript_cleanup branch 2 times, most recently from 0bc3cd9 to f9970b0 Compare March 28, 2025 12:04
@nivkner nivkner changed the title run frame scripts registered during frame script phase in a seperate pass core: run frame scripts registered during frame script phase in a seperate pass Mar 28, 2025
@nivkner nivkner changed the title core: run frame scripts registered during frame script phase in a seperate pass core: Run frame scripts registered during frame script phase in a seperate pass Mar 28, 2025
@nivkner nivkner changed the title core: Run frame scripts registered during frame script phase in a seperate pass core: Run scripts registered during frame script phase in a seperate pass Mar 28, 2025
@nivkner nivkner changed the title core: Run scripts registered during frame script phase in a seperate pass core: Run scripts registered in frame script phase in a seperate pass Mar 28, 2025
@nivkner nivkner changed the title core: Run scripts registered in frame script phase in a seperate pass core: Run scripts registered at frame script phase in a seperate pass Mar 28, 2025
@nivkner nivkner force-pushed the framescript_cleanup branch 3 times, most recently from b0aad6b to 66c73d2 Compare March 28, 2025 13:40
@nivkner
Copy link
Contributor Author

nivkner commented Mar 28, 2025

additional tests, attached here:
frame_script_test1.zip
frame_script_test2.zip
suggests that rather than inserting to the queue and then ignoring the script while traversing the tree normally,
there might actually be a deferred registration.
so if there is already a script in place, it gets executed during the traversal,
and the new one is executed during cleanup.

@nivkner
Copy link
Contributor Author

nivkner commented Mar 29, 2025

this test
frame_script_test3.zip
complicates matters, because while the script on the already registered frame does occur during regular traversal,
as well as the cleanup, the new script is executed.
in other words, the scripts are modified in place.

previous tests shown that scripts introduced during cleanup are not executed during traversal, when there were no scripts before. so next guess is that scripts registered during frame-script phase are enqueued for execution separately.

@nivkner
Copy link
Contributor Author

nivkner commented Mar 29, 2025

is there a scenario where the queued_script_frame field of MovieClip, when it is populated, is different from current_frame during frame script execution?
i replaced all usage of it with current_frame, and all the tests pass.

my current_plan is to replace it with has_pending_script boolean, which depend on the existence of a frame_script in the current frame, and can be toggled by the cleanup pass.

EDIT: never mind. will keep queued_script_frame.

@nivkner nivkner force-pushed the framescript_cleanup branch 2 times, most recently from 3150253 to 3ae6650 Compare March 29, 2025 14:18
@nivkner
Copy link
Contributor Author

nivkner commented Mar 29, 2025

fixed the behavior for the new test cases.

separated queued_script_frame into a boolean and a number, but no information should be lost (any case where the flag would be false, the script would skip, regardless of the value of the Option).

there are 2 cases of frame script behavior that i haven't resolved.
one is the aforementioned dynamic construction with buttons,
and the other is
frame_script_test4.zip
where, when using flash player, the frame script of Spawn seems to just disappear from the second cycle onward, (it doesn't reach the call to stop()).

since both are sufficiently different from the issue addressed by this PR, i will leave them for another time.

@nivkner nivkner force-pushed the framescript_cleanup branch 2 times, most recently from fa3b9a6 to 506fcda Compare March 29, 2025 21:28
@danielhjacobs danielhjacobs 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 Mar 31, 2025
@Lord-McSweeney
Copy link
Collaborator

#12942 might be fixed by this.

@nivkner nivkner force-pushed the framescript_cleanup branch from 506fcda to 2f8f340 Compare April 24, 2025 08:20
@nivkner
Copy link
Contributor Author

nivkner commented Apr 24, 2025

the PR does not fix #12942, confirmed by running against commit 2f8f340

@nivkner nivkner force-pushed the framescript_cleanup branch from 2f8f340 to 3f5d5b8 Compare April 28, 2025 18:15
@danielhjacobs danielhjacobs added the waiting-on-review Waiting on review from a Ruffle team member label May 2, 2025
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.

Assuming we're fine with this PR with no objections, it is well detailed, good job.
I ignored test files.

@nivkner nivkner force-pushed the framescript_cleanup branch from 3f5d5b8 to a1fc3d8 Compare May 18, 2025 19:25
@nivkner nivkner force-pushed the framescript_cleanup branch from a1fc3d8 to 2505a2b Compare May 18, 2025 19:34
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) waiting-on-review Waiting on review from a Ruffle team member
Projects
None yet
6 participants