-
-
Notifications
You must be signed in to change notification settings - Fork 932
avm1: Adjust clip removal logic for rewinds #21336
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,5 @@ | ||
| //! `MovieClip` display object and support code. | ||
| use crate::avm1::globals::AVM_DEPTH_BIAS; | ||
| use crate::avm1::Avm1; | ||
| use crate::avm1::{Activation as Avm1Activation, ActivationIdentifier}; | ||
| use crate::avm1::{NativeObject as Avm1NativeObject, Object as Avm1Object, Value as Avm1Value}; | ||
|
|
@@ -1562,10 +1563,15 @@ impl<'gc> MovieClip<'gc> { | |
| // TODO: We want to do something like self.children.retain here, | ||
| // but BTreeMap::retain does not exist. | ||
| // TODO: Should AS3 children ignore GOTOs? | ||
|
|
||
| let final_placements: HashMap<Depth, &GotoPlaceObject<'_>> = | ||
| goto_commands.iter().map(|cmd| (cmd.depth(), cmd)).collect(); | ||
|
|
||
| let children: SmallVec<[_; 16]> = self | ||
| .iter_render_list() | ||
| .filter(|clip| clip.place_frame() > frame) | ||
| .filter(|child| !self.survives_rewind(*child, &final_placements, frame)) | ||
| .collect(); | ||
|
|
||
| for child in children { | ||
| if !child.placed_by_script() { | ||
| self.remove_child(context, child); | ||
|
|
@@ -1690,6 +1696,83 @@ impl<'gc> MovieClip<'gc> { | |
| self.assert_expected_tag_end(hit_target_frame); | ||
| } | ||
|
|
||
| fn survives_rewind( | ||
| self, | ||
| old_object: DisplayObject<'_>, | ||
| final_placements: &HashMap<Depth, &GotoPlaceObject<'_>>, | ||
| frame: FrameNumber, | ||
| ) -> bool { | ||
| // TODO [KJ] This logic is not 100% tested. It's possible it's a bit | ||
| // different in reality, but the spirit is there :) | ||
|
|
||
| let is_candidate_for_removal = if self.movie().is_action_script_3() { | ||
| old_object.place_frame() > frame || old_object.placed_by_script() | ||
| } else { | ||
| old_object.depth() < AVM_DEPTH_BIAS | ||
| }; | ||
|
|
||
| if !is_candidate_for_removal && old_object.as_morph_shape().is_none() { | ||
| return true; | ||
| } | ||
| let Some(final_placement) = final_placements.get(&old_object.depth()) else { | ||
| return false; | ||
| }; | ||
|
|
||
| let new_params = &final_placement.place_object; | ||
|
|
||
| if !old_object.movie().is_action_script_3() | ||
| && old_object.placed_by_avm1_script() | ||
| && old_object.depth() < AVM_DEPTH_BIAS | ||
| { | ||
| return false; | ||
| } | ||
|
|
||
| let id_equals = match new_params.action { | ||
| swf::PlaceObjectAction::Place(id) | swf::PlaceObjectAction::Replace(id) => { | ||
| old_object.id() == id | ||
| } | ||
| _ => false, | ||
| }; | ||
|
|
||
| let ratio_equals = match new_params.ratio { | ||
| Some(ratio) => old_object.ratio() == ratio, | ||
| None => true, | ||
| }; | ||
|
Comment on lines
+1737
to
+1740
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Style nit: this could be shortened to
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's shorter but less readable, you'd have to think for a while what's the default value for
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Idk, it seems fairly natural to me: "
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The true/false default value is an equal choice, there's no reason why you would choose "is none or" vs "is some and". Matter of fact, for some properties it's the former, for some it's the latter. When trying to assess which one should be where, you have to map "is none or" & "is some and" to default values instead of reading them as-is. Not to mention, this logic is untested, so we don't really know which one should be where. |
||
|
|
||
| let clip_depth_equals = match new_params.clip_depth { | ||
| Some(clip_depth) => old_object.clip_depth() == clip_depth as Depth, | ||
| None => true, | ||
| }; | ||
|
|
||
| let color_transform_equals = match new_params.color_transform { | ||
| Some(color_transform) => old_object.base().color_transform() == color_transform, | ||
| None => true, | ||
| }; | ||
|
|
||
| let base_matrix_equals = match new_params.matrix { | ||
| Some(matrix) => old_object.base().matrix() == matrix.into(), | ||
| None => true, | ||
| }; | ||
|
|
||
| match old_object { | ||
| DisplayObject::MorphShape(_) | DisplayObject::Graphic(_) | DisplayObject::Text(_) => { | ||
| ratio_equals | ||
| && id_equals | ||
| && clip_depth_equals | ||
| && base_matrix_equals | ||
| && color_transform_equals | ||
| } | ||
| DisplayObject::Avm1Button(_) | ||
| | DisplayObject::Avm2Button(_) | ||
| | DisplayObject::EditText(_) | ||
| | DisplayObject::Bitmap(_) | ||
| | DisplayObject::Video(_) => ratio_equals && id_equals && clip_depth_equals, | ||
| DisplayObject::MovieClip(_) | ||
| | DisplayObject::Stage(_) | ||
| | DisplayObject::LoaderDisplay(_) => ratio_equals, | ||
| } | ||
| } | ||
|
|
||
| fn construct_as_avm1_object( | ||
| self, | ||
| context: &mut UpdateContext<'gc>, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1 @@ | ||
| num_frames = 6 | ||
| # FIXME - Ruffle doesn't handle AVM1 'goto's correctly | ||
| known_failure = true |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,3 @@ | ||
| # Test adapted from Shumway at https://github.com/mozilla/shumway/tree/master/test/swfs/avm1/duplicatemovieclip | ||
|
|
||
| num_frames = 4 | ||
| known_failure = true # https://github.com/ruffle-rs/ruffle/issues/12270 |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,3 @@ | ||
| # Test adapted from Shumway at https://github.com/mozilla/shumway/tree/master/test/swfs/timeline/nav | ||
|
|
||
| num_frames = 3 | ||
| known_failure = true # https://github.com/ruffle-rs/ruffle/issues/12143 |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,3 @@ | ||
| # Test adapted from Shumway at https://github.com/mozilla/shumway/tree/master/test/swfs/timeline/nav | ||
|
|
||
| num_frames = 3 | ||
| known_failure = true # https://github.com/ruffle-rs/ruffle/issues/12144 |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,3 @@ | ||
| # Test adapted from Shumway at https://github.com/mozilla/shumway/tree/master/test/swfs/timeline/nav | ||
|
|
||
| num_frames = 3 | ||
| known_failure = true # https://github.com/ruffle-rs/ruffle/issues/12145 | ||
kjarosh marked this conversation as resolved.
Show resolved
Hide resolved
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a larger refactor, we could instead have
goto_commandsbe directly aHashMap; from what I can see it doesn't have a meaningful ordering and we already ensure no duplicateDepths.