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: Implement flash.geom.PerspectiveProjection #19532

Merged
merged 3 commits into from
Feb 24, 2025

Conversation

cookie-s
Copy link
Contributor

@cookie-s cookie-s commented Feb 16, 2025

doc: https://docs.ruffle.rs/en_US/FlashPlatform/reference/actionscript/3/flash/geom/PerspectiveProjection.html

This PR implements flash.geom.PerspectiveProjection features. However, assignment to flash.display.DisplayObject through transform.perspectiveProjection is not yet supported. No effect to visual, either.

@cookie-s cookie-s force-pushed the avm2-perspective-projection branch 3 times, most recently from 21ae2a2 to de64c8d Compare February 16, 2025 02:15
@cookie-s cookie-s marked this pull request as ready for review February 16, 2025 02:28
@cookie-s cookie-s force-pushed the avm2-perspective-projection branch 2 times, most recently from e929ede to afc763f Compare February 16, 2025 02:51
Comment on lines +17 to +21
// Not associated with any DO
None => 500.0,
// Stage's PerspectiveProjection
Some(dobj) if dobj.as_stage().is_some() => 500.0,
Copy link
Contributor Author

@cookie-s cookie-s Feb 16, 2025

Choose a reason for hiding this comment

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

I don't know where this value 500 comes from. It fits the output from the test in FP.

private var displayObject: DisplayObject = null;

[Ruffle(NativeAccessible)]
private var fov: Number = 55.0;
Copy link
Contributor Author

@cookie-s cookie-s Feb 16, 2025

Choose a reason for hiding this comment

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

From TestTransform() result, fieldOfView should be saved and focalLength should be computed from it.
PP(FOV:55, FL:480.25) assignment to Sprite.transform.perspectiveProjection produces PP(FOV:55, FL:288.15) (when SWF width is 300).

@cookie-s cookie-s force-pushed the avm2-perspective-projection branch from afc763f to 2aa2853 Compare February 16, 2025 04:26
@emergence75
Copy link

Copy link
Collaborator

@adrian17 adrian17 left a comment

Choose a reason for hiding this comment

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

I think it all looks fine, but if you don’t mind, I wanna investigate a bit into the remaining unknowns; but I’ll only be able to do it on wednesday.

stub_method("flash.geom.PerspectiveProjection", "toMatrix3D");
return new Matrix3D();
var fl: Number = this.focalLength;
return new Matrix3D(new <Number>[
Copy link
Collaborator

@adrian17 adrian17 Feb 16, 2025

Choose a reason for hiding this comment

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

To be sure, this is a complete matrix impl? Asking due to the removed stub.

Copy link
Contributor Author

@cookie-s cookie-s Feb 17, 2025

Choose a reason for hiding this comment

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

Yes, I believe so. For other methods, especially setters are related to unimplemented rendering/visual, but I believe this toMatrix3D just returns the Matrix3D object.

Ah, but only if the small but not very small floating point number differences are allowed. Please see precision in TestToMatrix3D test function. There is unexplainable numerical errors which is now rounded to approximation in the test.

Except for that, I believe all features in toMatrix3D are implemented.

@adrian17
Copy link
Collaborator

adrian17 commented Feb 16, 2025

As for emergence75’s code, note that we did find issues with some of the changes there (see: #18495 (comment) and the subsequent comment). I know that since then it was replaced by a newer PR (which I’m sorry for not commenting on), but as far as I’m aware, the listed concerns about the changes still apply :( With that, I’d be careful with treating it as a source.

@kjarosh kjarosh added A-avm2 Area: AVM2 (ActionScript 3) T-compat Type: Compatibility with Flash Player labels Feb 16, 2025
@cookie-s
Copy link
Contributor Author

this might be useful, take a look

Oh thank you. My PR might be better to implement the input validation for fieldOfView setter.

I wanna investigate a bit into the remaining unknowns

Sure thank you!

@cookie-s cookie-s force-pushed the avm2-perspective-projection branch from 2aa2853 to 3fadf21 Compare February 18, 2025 13:45
// Stage's PerspectiveProjection
Some(dobj) if dobj.as_stage().is_some() => 500.0,
// root's PerspectiveProjection
Some(dobj) if dobj.is_root() => activation.context.stage.stage_size().0 as f64,
Copy link
Collaborator

Choose a reason for hiding this comment

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

If this has the same logic as the one below, any reason for it to be separate?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. No! Thank you, merged to the line below at f6f2aba

import flash.geom.Matrix3D;
import flash.geom.Point;

public class PerspectiveProjection {
// Getters are stubbed with what seem to be Flash's default values
[Ruffle(NativeAccessible)]
private var displayObject: DisplayObject = null;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this actually need the live reference to the DO? What if instead, only the width value was stored?

Copy link
Contributor Author

@cookie-s cookie-s Feb 20, 2025

Choose a reason for hiding this comment

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

True, storing only width passes the current test, but another observation by for example this code:

        private function TestTransformUpdate(): void {
            var s: Sprite = new Sprite();

            trace("// Set non-null to transform.perspectiveProjection");
            s.transform.perspectiveProjection = new PerspectiveProjection();
            printProps(s.transform.perspectiveProjection);

            trace("// Set null to transform.perspectiveProjection");
            s.transform.perspectiveProjection.fieldOfView = 100;
            printProps(s.transform.perspectiveProjection);
        }

shows some setter in DO.transform.perspectiveProjection : PerspectiveProjection should update the values in DO.

It's the similar behavior to transform.matrix3D rather than transform.matrix in terms of what we discussed before: #18810 (comment)
(although that setter behavior is not implemented to matrix3D and transform.perspectiveProjection setter is almost imcomplete as of now.)

Given that, we will anyway need reference to DO at some point I believe.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, I see, yeah.

            var a = dobj.transform.perspectiveProjection;
            var b = dobj.transform.perspectiveProjection;
            trace(a === b); // false
            a.fieldOfView = 123;
            trace(b.fieldOfView); // 123

Copy link
Collaborator

@adrian17 adrian17 Feb 20, 2025

Choose a reason for hiding this comment

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

Can you please add some TODO comments explaining the above, and maybe a known-failure test? But just a comment is enough, I think I'm gonna approve the PR in a moment.

although that setter behavior is not implemented to matrix3D and transform.perspectiveProjection setter is almost imcomplete as of now

Sure, I think this is an okay compromise.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you! I added a few TODO/FIXME comments.

@cookie-s cookie-s force-pushed the avm2-perspective-projection branch from 3fadf21 to f5e13c0 Compare February 20, 2025 12:57
@cookie-s cookie-s force-pushed the avm2-perspective-projection branch from f5e13c0 to dd45db7 Compare February 21, 2025 13:19
@adrian17 adrian17 force-pushed the avm2-perspective-projection branch from dd45db7 to 17be770 Compare February 23, 2025 19:03
@adrian17 adrian17 force-pushed the avm2-perspective-projection branch from 17be770 to 0994108 Compare February 24, 2025 23:00
@adrian17 adrian17 enabled auto-merge (rebase) February 24, 2025 23:00
@adrian17
Copy link
Collaborator

Thanks!

(as for the z-order PR, I'm trying to find time to take a look at it, sorry for a delay 😅 )

@adrian17 adrian17 merged commit ae95842 into ruffle-rs:master Feb 24, 2025
22 checks passed
cookie-s added a commit to cookie-s/ruffle that referenced this pull request Feb 25, 2025
ae3d5f4 (ruffle-rs#19532) found the form of flash.geom.PerspectiveProjection
toMatrix3D() method output. Update Ruffle's rendering matrix to follow
the same.
cookie-s added a commit to cookie-s/ruffle that referenced this pull request Feb 25, 2025
ae3d5f4 (ruffle-rs#19532) found the form of flash.geom.PerspectiveProjection
toMatrix3D() method output. Update Ruffle's rendering matrix to follow
the same.
cookie-s added a commit to cookie-s/ruffle that referenced this pull request Feb 25, 2025
ae3d5f4 (ruffle-rs#19532) found the form of flash.geom.PerspectiveProjection
toMatrix3D() method output. Update Ruffle's rendering matrix to follow
the same.
@cookie-s
Copy link
Contributor Author

cookie-s commented Feb 25, 2025

Thank you for merging!

(No problem at all, I know it's not a tiny PR. I have just pushed my last update before its review.)

@cookie-s cookie-s deleted the avm2-perspective-projection branch February 25, 2025 13:14
Hancock33 added a commit to Hancock33/batocera.piboy that referenced this pull request Feb 26, 2025
-----------------------------------------------------------------
libfreeaptx.mk dae97a4df4181fae364bf7ae2a1ea6ecd76968a0 NULL-NULL
-----------------------------------------------------------------
Moved Permanently,

--------------------------------------------------------------------------------------------------------
batocera-emulationstation.mk a3c573111285ffc96c8ec2f32b83a4d101181b51 # Version: Commits on Feb 25, 2025
--------------------------------------------------------------------------------------------------------
update po

Signed-off-by: Nicolas Adenis-Lamarre <nicolas.adenis.lamarre@gmail.com>,

------------------------------------------------------------------------------------------------
batocera-es-piboy.mk a3c573111285ffc96c8ec2f32b83a4d101181b51 # Version: Commits on Feb 25, 2025
------------------------------------------------------------------------------------------------
update po

Signed-off-by: Nicolas Adenis-Lamarre <nicolas.adenis.lamarre@gmail.com>,

------------------------------------------------------------------------------------------
dolphin-emu.mk 030892abd981f661a5a272194089552122ddc331 # Version: Commits on Feb 25, 2025
------------------------------------------------------------------------------------------
Merge pull request #13369 from Tilka/qt_warnings

CMake: disable Qt deprecation warnings we can't fix,

-----------------------------------------------------------------------------------------
lightspark.mk 2cc6e74ea11182a71b3a1902da2cadbc004a9063 # Version: Commits on Feb 25, 2025
-----------------------------------------------------------------------------------------
Merge branch 'master' of github.com:lightspark/lightspark,

----------------------------------------------------
pcsx2.mk v2.3.182 # Version: Commits on Feb 25, 2025
----------------------------------------------------
- [GameDB: Add Namco CRC to Tales o.t. Abyss + Taiko no Tatsujin 9](PCSX2/pcsx2#12346)

-------------------------------------------------------------------------------------
ppsspp.mk 805816da1de0b6dd56f290c0d0467aadc3170617 # Version: Commits on Feb 25, 2025
-------------------------------------------------------------------------------------
Merge pull request #20033 from hrydgard/driver-icon-fix

Use libpng's full API so we can ignore gamma. Fixes Driver '76's icon.,

---------------------------------------------------------------
ruffle.mk nightly-2025-02-25 # Version: Commits on Feb 25, 2025
---------------------------------------------------------------
## What's Changed

* chore: Update translations by @RuffleBuild in ruffle-rs/ruffle#19611

* avm2: Implement flash.geom.PerspectiveProjection by @cookie-s in ruffle-rs/ruffle#19532

**Full Changelog**: ruffle-rs/ruffle@nightly-2025-02-24...nightly-2025-02-25,

--------------------------------------------------------------------------------------
shadps4.mk 15d10e47ea272b1b4c8bf97f2b3bbb406d34b213 # Version: Commits on Feb 24, 2025
--------------------------------------------------------------------------------------
fix: move trophy pngs to src/images (#2519)

* fix: move trophy pngs to src/images

* Fix pointers to trophy files

---------

Co-authored-by: rainmakerv2 <30595646+jpau02@users.noreply.github.com>,

-------------------------------------------------------------------------------------
snes9x.mk 0525ea043ea173ec6af1c6c8dade820036376430 # Version: Commits on Feb 25, 2025
-------------------------------------------------------------------------------------
Merge pull request #969 from yeager/master

Adding Swedish translation,

-------------------------------------------------
vice.mk r45507 # Version: Commits on Feb 25, 2025
-------------------------------------------------
XDG desktop files: use `$(DESTDIR)${prefix}` for installation

As suggested by Rhailto, use `--dir-=$(DESTDIR)${prefix}/share/applications` as

argument to `desktop-file-install` to support

ormal\ installation of .desktop

files through `make install` and allow for staged installs. Update the rules

to remove the .desktop files on `make uninstall` in the same vein.

With the default prefix of `/usr/local` that VICE's configure uses, the .desktop

files end up in `/usr/local/share/applications`, a directory that didn't exist

on my Debian system with Gnome, but which is created by `desktop-file-install`

and the .desktop files are processed by `update-desktop-database` and VICE's

application icons show up in the Gnome application overview and correctly launch

the emulators.

Note the use of `${prefix}` rather than `$(PREFIX)`, the later was empty on my

`make install`, resulting in .desktop files in `/share/applications` in the

root file system =)

git-svn-id: https://svn.code.sf.net/p/vice-emu/code/trunk@45507 379a1393-f5fb-40a0-bcee-ef074d9b53f7,

------------------------------------------------------------------------------------
box64.mk 48f51bd5fef5b9b6ea5ad086cfd44a07c5441d8a # Version: Commits on Feb 25, 2025
------------------------------------------------------------------------------------
[BOX32][WRAPPER] Use a temp xcb when converting an unknown xcb_connection instead of adding it,

------------------------------------------------------------------------------------------
devilutionx.mk 8dd1ddc65bbe6ce55a88d0f6a5a87eb9be9beb53 # Version: Commits on Feb 25, 2025
------------------------------------------------------------------------------------------
Update ru.po (#7785),

---------------------------------------------------
nblood.mk r14198 # Version: Commits on Feb 25, 2025
---------------------------------------------------
-

----------------------------------------------------------------------------------------
supertux2.mk e4aebedbeab6d761449a2424343fd15b13b6fafc # Version: Commits on Feb 24, 2025
----------------------------------------------------------------------------------------
PathWalker: Small code style fix

[ci skip],

---------------------------------------------------------------------------------------------
easyrpg-player.mk 6217ceb83ff1c3708a6a34dcb62fecc53eb6ac14 # Version: Commits on Feb 25, 2025
---------------------------------------------------------------------------------------------
Merge pull request #3343 from EasyRPG-NewFeatures/fix-face-OOB

Fix OOB reads,

----------------------------------------------------------------------------------------
retroarch.mk e16953b9afbec278c43874132f24f0ff39b6852c # Version: Commits on Feb 25, 2025
----------------------------------------------------------------------------------------
Fetch translations from Crowdin,

----------------------------------------------------------------------------------------
doomretro.mk 4e7a48d8e15c3f71d25cc8fee13c69ae7cd610ca # Version: Commits on Feb 25, 2025
----------------------------------------------------------------------------------------
Remove more D4V code,

-------------------------------------------------------------------------------------
gzdoom.mk 52a54521ed43d2495cab3e194e7f8d55bca799b1 # Version: Commits on Feb 25, 2025
-------------------------------------------------------------------------------------
- fix erroneous check in previous commit,

-----------------------------------------------------------------------------------
tr1x.mk f98d5a35743e0da9915dea8073403579561160e2 # Version: Commits on Feb 25, 2025
-----------------------------------------------------------------------------------
tr2/inventory: fix flare pickup quantity

This ensures we always add 6 flares when Lara picks up item 151. The

other logic for single flares is required for gameflow flare additions,

like in Great Wall.

Resolves #2551.,

-----------------------------------------------------------------------------------
tr2x.mk f98d5a35743e0da9915dea8073403579561160e2 # Version: Commits on Feb 25, 2025
-----------------------------------------------------------------------------------
tr2/inventory: fix flare pickup quantity

This ensures we always add 6 flares when Lara picks up item 151. The

other logic for single flares is required for gameflow flare additions,

like in Great Wall.

Resolves #2551.,

------------------------------------------------------------------------------------------
xash3d-fwgs.mk f1f726822cc8e8427471b865d18d63180c5dda08 # Version: Commits on Feb 25, 2025
------------------------------------------------------------------------------------------
engine: client: restore cl_cmdrate default value at 30. It wasn't an intentional change.,

-----------------------------------------------------------------------------------------------
libretro-easyrpg.mk 6217ceb83ff1c3708a6a34dcb62fecc53eb6ac14 # Version: Commits on Feb 25, 2025
-----------------------------------------------------------------------------------------------
Merge pull request #3343 from EasyRPG-NewFeatures/fix-face-OOB

Fix OOB reads,

---------------------------------------------------------------------------------------------
libretro-fbneo.mk 84cfa021b0246460250c904bbeea5b52a57b2ea6 # Version: Commits on Feb 25, 2025
---------------------------------------------------------------------------------------------
(libretro) update files,

-----------------------------------------------------------------------------------------------------
libretro-mame2003-plus.mk 558ef7258f3e12bb191f1e4ed44039f25f7d762b # Version: Commits on Feb 25, 2025
-----------------------------------------------------------------------------------------------------
cyclone: use HAVE_ARMv6 flag for backward asm compatibility (#1922),

----------------------------------------------------------------------------------------------
libretro-ppsspp.mk 805816da1de0b6dd56f290c0d0467aadc3170617 # Version: Commits on Feb 25, 2025
----------------------------------------------------------------------------------------------
Merge pull request #20033 from hrydgard/driver-icon-fix

Use libpng's full API so we can ignore gamma. Fixes Driver '76's icon.,

--------------------------------------------------------------------------------------------
slang-shaders.mk c511dcf0bf97b98852bc078eb9f5ca7e2ffe45f9 # Version: Commits on Feb 25, 2025
--------------------------------------------------------------------------------------------
sync to koko-aio-1.9.63-HotFix3 (#690)

* Sync to koko-aio-HotFix2

* sync to koko-aio-HotFix3,
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-avm2 Area: AVM2 (ActionScript 3) T-compat Type: Compatibility with Flash Player
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants