avm2: Store PerspectiveProjection to DisplayObject #19670

cookie-s · 2025-03-01T17:56:39Z

Continued from #19532 .

DisplayObject stores Option<PerspectiveProjection> through Transform so we can remove HAS_PERSPECTIVE_PROJECTION_STUB flag from display objects.

The test from_shumway/avm2/flash/geom/perspectiveprojection passes now.

Limitation

PerspectiveProjection is stored in Transform, but it's not used in rendering at all. (Hence, we cannot remove stub_setters)

cookie-s · 2025-04-26T14:27:52Z

tests/tests/swfs/from_shumway/avm2/flash/geom/perspectiveprojection/test.toml

+number_patterns = [
+# Both FP 32,0,0,465 (for Linux) and FP 32 (for Windows) produced slightly different numbers in some environment. However, the original trace from mozilla/shumway is respected in the output file.
+    'focalLength: ([0-9.]+)'
+]
+max_relative = 0.0000001


I believe the output.txt is from https://github.com/mozilla/shumway/blob/master/test/swfs/avm2/flash/geom/perspectiveprojection/PerspectiveProjectionClass.swf.trace .
I couldn't reproduce exactly the same result from PerspectiveProjectionClass.as in the same directory (after recompilation with mxmlc) or even from the test.swf (without recompilation).

(Fun fact: I was able to pass this test without this approximation until #19748 was merged -- What does it mean? Anyway, I think the problem is that I cannot reproduce the output.txt from ActionScript or SWF.)

I think a small difference like this isn't a problem, we have similar issues with some of the AVM2 Number tests already.

core/src/avm2/globals/flash/geom/perspective_projection.rs

core/src/avm2/globals/flash/geom/transform.rs

Lord-McSweeney · 2025-04-29T22:34:05Z

core/src/avm2/globals/flash/geom/transform.rs

            .avm2()
            .classes()
            .perspectiveprojection
-            .construct(activation, &[])?;
+            .construct(activation, &[])?


Hmm, does this really return a new PerspectiveProjection each time the getter is called?

Yes, to make it clear, I added a test case testEq() to geom_transform just now.
From the same Transform, every call to the getter creates a new PerspectiveProjection.

Lord-McSweeney · 2025-04-29T22:36:08Z

core/src/display_object/stage.rs

@@ -193,6 +194,7 @@ impl<'gc> Stage<'gc> {
            },
        ));
        stage.set_is_root(gc_context, true);
+        stage.set_perspective_projection(gc_context, None); // Set default PerspectiveProjection


Is this line necessary?

Yes, this line actually sets PerspectiveProjection::default() to DO's PerspectiveProjection.

ruffle/core/src/display_object/stage.rs

Lines 894 to 896 in aa6197e

if perspective_projection.is_none() {

// `stage` doesn't allow null PerspectiveProjection.

perspective_projection = Some(Default::default());

Technically it's fine to set PerspectiveProjection::default() directly at here, but for consistency with root.set_perspective_projection() in a different file I'm passing None here.

ruffle/core/src/context.rs

Line 397 in aa6197e

root.set_perspective_projection(self.gc(), None); // Set default PerspectiveProjection

The root actually has a different value used when None (null) is provided..

ruffle/core/src/display_object/movie_clip.rs

Lines 2836 to 2845 in aa6197e

if perspective_projection.is_none() && self.is_root() {

// `root` doesn't allow null PerspectiveProjection.

perspective_projection = Some(PerspectiveProjection {

field_of_view: 55.0,

center: (

self.movie().width().to_pixels() / 2.0,

self.movie().height().to_pixels() / 2.0,

),

});

}

cookie-s

Thank you for review!

core/src/avm2/globals/flash/geom/perspective_projection.rs

cookie-s · 2025-05-02T00:57:43Z

core/src/avm2/globals/flash/geom/transform.rs

            .avm2()
            .classes()
            .perspectiveprojection
-            .construct(activation, &[])?;
+            .construct(activation, &[])?


Yes, to make it clear, I added a test case testEq() to geom_transform just now.
From the same Transform, every call to the getter creates a new PerspectiveProjection.

cookie-s · 2025-05-02T01:07:02Z

core/src/display_object/stage.rs

@@ -193,6 +194,7 @@ impl<'gc> Stage<'gc> {
            },
        ));
        stage.set_is_root(gc_context, true);
+        stage.set_perspective_projection(gc_context, None); // Set default PerspectiveProjection


Yes, this line actually sets PerspectiveProjection::default() to DO's PerspectiveProjection.

ruffle/core/src/display_object/stage.rs

Lines 894 to 896 in aa6197e

if perspective_projection.is_none() {

// `stage` doesn't allow null PerspectiveProjection.

perspective_projection = Some(Default::default());

Technically it's fine to set PerspectiveProjection::default() directly at here, but for consistency with root.set_perspective_projection() in a different file I'm passing None here.

ruffle/core/src/context.rs

Line 397 in aa6197e

root.set_perspective_projection(self.gc(), None); // Set default PerspectiveProjection

The root actually has a different value used when None (null) is provided..

ruffle/core/src/display_object/movie_clip.rs

Lines 2836 to 2845 in aa6197e

if perspective_projection.is_none() && self.is_root() {

// `root` doesn't allow null PerspectiveProjection.

perspective_projection = Some(PerspectiveProjection {

field_of_view: 55.0,

center: (

self.movie().width().to_pixels() / 2.0,

self.movie().height().to_pixels() / 2.0,

),

});

}

core/src/avm2/globals/flash/geom/transform.rs

Stage and root have their own initial PerspectiveProjection value, and it's also used when attempting to set `null`.

* Remove FIXME for TestTranformUpdate() * Add four more test cases

Small number differences in focalLength are accepted as approximation

It's now unused because ruffle_render::Transform stores Option<PerspectiveProjection>.

It's very hard to verify this value with any test while PerspectiveProjection rendering is not implemented. However, this change makes the most sense.

It's not obvious whether each getter of one Transform object should return the same instance or newly created instance.

cookie-s force-pushed the perspective-projection-storage branch 7 times, most recently from 19168c2 to af96e50 Compare March 2, 2025 17:43

cookie-s marked this pull request as ready for review March 2, 2025 17:56

Lord-McSweeney added A-avm2 Area: AVM2 (ActionScript 3) T-fix Type: Bug fix (in something that's supposed to work already) waiting-on-review Waiting on review from a Ruffle team member labels Mar 3, 2025

cookie-s force-pushed the perspective-projection-storage branch from af96e50 to 970b60c Compare April 26, 2025 13:19

cookie-s marked this pull request as draft April 26, 2025 13:23

cookie-s force-pushed the perspective-projection-storage branch 3 times, most recently from e5874ce to da3e09a Compare April 26, 2025 14:17

cookie-s commented Apr 26, 2025

View reviewed changes

cookie-s marked this pull request as ready for review April 26, 2025 14:31

Lord-McSweeney reviewed Apr 29, 2025

View reviewed changes

core/src/avm2/globals/flash/geom/perspective_projection.rs Show resolved Hide resolved

Lord-McSweeney reviewed Apr 29, 2025

View reviewed changes

core/src/avm2/globals/flash/geom/transform.rs Outdated Show resolved Hide resolved

Lord-McSweeney reviewed Apr 29, 2025

View reviewed changes

Lord-McSweeney approved these changes Apr 29, 2025

View reviewed changes

cookie-s force-pushed the perspective-projection-storage branch from da3e09a to aa6197e Compare May 2, 2025 00:42

cookie-s commented May 2, 2025

View reviewed changes

cookie-s force-pushed the perspective-projection-storage branch from aa6197e to 513015d Compare May 11, 2025 02:25

cookie-s added 4 commits May 27, 2025 03:17

render: Define PerspectiveProjection struct into Transform

de8be6b

avm2: Implement PerspectiveProjection getters/setters to DO

1ede9d0

avm2: Implement special PerspectiveProjection for Stage and root

6da5290

Stage and root have their own initial PerspectiveProjection value, and it's also used when attempting to set `null`.

tests: Add tests for perspective_projection setters

5a92253

* Remove FIXME for TestTranformUpdate() * Add four more test cases

cookie-s force-pushed the perspective-projection-storage branch 2 times, most recently from fdba546 to 744856c Compare May 28, 2025 00:24

cookie-s added 4 commits May 28, 2025 00:24

tests: Unmark known_failure from from_shumway/perspectiveprojection test

e624855

Small number differences in focalLength are accepted as approximation

core: Remove HAS_PERSPECTIVE_PROJECTION_STUB flag

ce0b9ad

It's now unused because ruffle_render::Transform stores Option<PerspectiveProjection>.

core: Fill Stage's perspective_projection in render()

60b626a

It's very hard to verify this value with any test while PerspectiveProjection rendering is not implemented. However, this change makes the most sense.

test: Add testEq testcase to geom_transform

744856c

It's not obvious whether each getter of one Transform object should return the same instance or newly created instance.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

avm2: Store PerspectiveProjection to DisplayObject #19670

avm2: Store PerspectiveProjection to DisplayObject #19670

cookie-s commented Mar 1, 2025 •

edited

Loading

Uh oh!

cookie-s Apr 26, 2025

Uh oh!

Lord-McSweeney Apr 29, 2025

Uh oh!

Uh oh!

Uh oh!

Lord-McSweeney Apr 29, 2025

Uh oh!

cookie-s May 2, 2025

Uh oh!

Lord-McSweeney Apr 29, 2025

Uh oh!

cookie-s May 2, 2025

Uh oh!

cookie-s left a comment

Uh oh!

Uh oh!

cookie-s May 2, 2025

Uh oh!

cookie-s May 2, 2025

Uh oh!

Uh oh!

Uh oh!

	if perspective_projection.is_none() {
	// `stage` doesn't allow null PerspectiveProjection.
	perspective_projection = Some(Default::default());

	if perspective_projection.is_none() && self.is_root() {
	// `root` doesn't allow null PerspectiveProjection.
	perspective_projection = Some(PerspectiveProjection {
	field_of_view: 55.0,
	center: (
	self.movie().width().to_pixels() / 2.0,
	self.movie().height().to_pixels() / 2.0,
	),
	});
	}

Uh oh!

avm2: Store PerspectiveProjection to DisplayObject #19670

Are you sure you want to change the base?

avm2: Store PerspectiveProjection to DisplayObject #19670

Conversation

cookie-s commented Mar 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Limitation

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cookie-s left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

cookie-s commented Mar 1, 2025 •

edited

Loading