Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
85 changes: 84 additions & 1 deletion core/src/display_object/movie_clip.rs
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};
Expand Down Expand Up @@ -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();

Comment on lines +1566 to +1569
Copy link
Contributor

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_commands be directly a HashMap; from what I can see it doesn't have a meaningful ordering and we already ensure no duplicate Depths.

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);
Expand Down Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

Style nit: this could be shortened to new_params.ratio.is_none_or(|r| r == old_object.ratio()) (and similarly for the subsequent matches).

Copy link
Member

Choose a reason for hiding this comment

The 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 None, it's not natural to read it as is none or / is some and.

Copy link
Contributor

@moulins moulins Oct 11, 2025

Choose a reason for hiding this comment

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

Idk, it seems fairly natural to me: "new_params.ratio is none or equal to old_object.ratio()"

Copy link
Member

Choose a reason for hiding this comment

The 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>,
Expand Down
2 changes: 0 additions & 2 deletions tests/tests/swfs/avm1/rewind_depth/test.toml
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
Loading