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

AVM2: RemoveObject runs at the start of a frame & frame phase tracking #7048

Closed
wants to merge 9 commits into from

Conversation

kmeisthax
Copy link
Member

This PR does four things:

  • Moves RemoveObject handling to the enter_frame part of frame processing, because it happens before the EnterFrame event is emitted.
  • Adds a mechanism for tracking what phase of frame processing we are in. This isn't super-useful to this PR but it will be absolutely necessary for the goto/loop stuff in AVM2 Frame Events Ordering Fixes #5876.
  • Moves all the extant enter_frame/construct_frame calls to one place. This also demonstrates how the frame phase tracking can be used to avoid sending events to objects too early or too late.
  • Ignores avm2_simplebutton_constr_childevents because it is reporting an extra accidental EnterFrame event. This doesn't happen in the PR I cut this off from, but I don't know why. Best guess is that something in [DRAFT] AVM2: Fixes for SimpleButton #7015 fixes it.

The test output for this test is sensitive to where we cut off each frame, because it doesn't stop all of it's handlers at the end of the test. Flash Player will just print lines forever so the end of the test is entirely arbitrary.
…ame` into a single method, `catchup_display_object_to_frame`.

The rationale for the catch-up logic is as follows:

 * We must always enter-frame and construct objects, even if those respective display events haven't happened yet.
 * Display objects created in event handlers still need to run catchup phases, otherwise they will tag-stream desync
 * Frame scripts are never triggered by catchup phases
 * `exit_frame` is not a catchup phase as it is *only* an event broadcast currently

if is_playing {
// Frame destruction happens in-line with frame number advance.
// If we expect to loop, we do not run `RemoveObject` tags.
Copy link
Member

Choose a reason for hiding this comment

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

This is talking about looping back to the first frame, not 'looping' on the current frame, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes.

@Herschel
Copy link
Member

Herschel commented Jul 25, 2022

Following from Discord discussion, I'm a little skeptical that FP would do multiple passes over the SWF tag stream per frame. I'd imagine it would do one pass over the tag stream, executing all of the tags, but deferring event dispatch until it knows exactly what the final state of the frame is.

Here's an example SWF that does a PlaceObject of a clip, followed by RemoveObject of that clip, on a single frame:
test-add-remove.zip
In the Flash Player, no added or removed events are fired, which hints that these events don't actually get dispatched until the tags are resolved for that frame, and we know which objects will actually exist in the end.
In this PR, the SWF ends up incorrectly displaying the clip, because the RemoveObject runs first -- so it seems to me that the premise of RemoveObject running at the start of the frame isn't right.
In master, the added/removed events incorrectly fire, but the object is correctly never displayed.

I admittedly don't have the full picture here, but I feel like deferring the event execution in this way would make the code simpler, allow us to possibly get all of the tag handling back into a single fn, and clean up a lot of the special cases.

@kmeisthax
Copy link
Member Author

I'm going to prototype the "read tag stream in enter_frame and maintain a queue of outstanding actions" approach (on a different branch, of course) and see how far I can get with the test suite. Theoretically, there shouldn't be any reason why this won't work, and the only major unknown is if the queue is local or global and how it interacts with gotos.

@Herschel
Copy link
Member

This was superseded by #7570.

@Herschel Herschel closed this Aug 27, 2022
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.

3 participants