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 Matrix3D with 2D support only #18888

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

cookie-s
Copy link
Contributor

@cookie-s cookie-s commented Dec 7, 2024

Retry #18810 .
Partially resolves #8033 .

#18810 (comment)

To be merged again, it will need a test + the patch posted by me above + 2D/3D switch

Commits

@cookie-s cookie-s force-pushed the matrix3d-transformation-with-2d-only branch from 7473740 to ac547ca Compare December 7, 2024 00:35
@cookie-s cookie-s changed the title Matrix3d transformation with 2d only avm2: Implement Matrix3D with 2D support only Dec 7, 2024
@kjarosh kjarosh added A-avm2 Area: AVM2 (ActionScript 3) T-compat Type: Compatibility with Flash Player labels Dec 7, 2024
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This log is taken from Adobe Flash Player 32.

tests/tests/swfs/avm2/geom_transform/output.expected.png Outdated Show resolved Hide resolved
@cookie-s cookie-s marked this pull request as ready for review December 7, 2024 01:03
@cookie-s cookie-s force-pushed the matrix3d-transformation-with-2d-only branch from ac547ca to 69f6554 Compare December 10, 2024 01:37
@cookie-s
Copy link
Contributor Author

#18908 produced Git conflict with this PR and it actually implemented a kind of "2D/3D switch" inside DisplayObject. Rebased and followed its design now.

@cookie-s cookie-s force-pushed the matrix3d-transformation-with-2d-only branch 3 times, most recently from a3a3018 to 6417e5f Compare December 14, 2024 00:00
@cookie-s
Copy link
Contributor Author

cookie-s commented Dec 14, 2024

Rebased. Thanks to #18911 , this PR gets much more slim.

@cookie-s cookie-s force-pushed the matrix3d-transformation-with-2d-only branch from 796ced1 to 35413ad Compare December 21, 2024 00:35
@danielhjacobs danielhjacobs added the waiting-on-review Waiting on review from a Ruffle team member label Dec 26, 2024
Replace stubs for flash.geom.Transform.matrix3D getter/setter with an
actual implementation of Matrix3D with limited support.

This implementation is just a proxy to the existing 2D matrix
implementation. Therefore transformations beyond 2D transformation works
differently from the expected result.
@cookie-s cookie-s force-pushed the matrix3d-transformation-with-2d-only branch from 35413ad to 236c1cb Compare December 27, 2024 03:05
@cookie-s
Copy link
Contributor Author

Rebased.

@@ -1,7 +1,8 @@
num_frames = 1

[image_comparisons.output]
tolerance = 0
tolerance = 10
max_outliers = 4000

[player_options]
with_renderer = { optional = true, sample_count = 1 }
Copy link
Member

Choose a reason for hiding this comment

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

I believe the FP screenshot was done using high quality, in which case the sample count should be 4. This will hopefully make images closer to each other (so that we can possibly drop max outliers)

Also this commit can be safely squashed with the previous test commit

Copy link
Contributor Author

@cookie-s cookie-s Dec 28, 2024

Choose a reason for hiding this comment

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

Yes, I took the screenshot with high quality. I see, I didn't know the meaning of sample_count. Now fixed and squashed to 770d63e !

However, max_outliers cannot be dropped so much
Image 'output' succeeded: 3690 outliers found, max difference 255

Here is the diff created by imgtests:
output difference-color-linux-Gl
The edges of the objects may be slightly different?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, you're right. I've been trying to debug it but it seems that Flash just renders those objects differently with Matrix3D compared to Matrix, even if they represent the same transformation. I think we have to leave high max outliers and just add FIXME/TODO above it which describes the issue

Copy link
Contributor Author

@cookie-s cookie-s Dec 30, 2024

Choose a reason for hiding this comment

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

Oh, I didn't notice that 2D and 3D had the difference. True. Thank you, I added a comment: 81de5b6#diff-1cc83ff3bc64c1ef783edf3b104e090e3ea173a7298b6a3322ccdd7538fe80ecR5

trace("sprite3D.transform.matrix3D.rawData", sprite3D.transform.matrix3D.rawData);
trace("mat3D.rawData", mat3D.rawData);

//// FIXME: matrix3D.rawData should be updated by x/y update.
Copy link
Member

Choose a reason for hiding this comment

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

I believe that's already the case, it's just that we don't reset tx,ty on matrix3D = null and the assertions below fail.

We could also do some more assertions related to non-indentity 3D matrices, I think we can safely do that without modifying the code

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. You're right. I added more test cases for non-identity matrices plus uncomment: 1b6802d

And thanks to the test case, I found another interesting behavior. When setting .transform.matrix3D = null, the matrix gets reset to the identity matrix. Added a fix for that: 019476d

@cookie-s cookie-s force-pushed the matrix3d-transformation-with-2d-only branch 2 times, most recently from f17cb03 to 019476d Compare December 28, 2024 01:38
@cookie-s cookie-s requested a review from kjarosh December 28, 2024 02:00
@cookie-s cookie-s force-pushed the matrix3d-transformation-with-2d-only branch from 019476d to af60217 Compare December 29, 2024 23:52
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 waiting-on-review Waiting on review from a Ruffle team member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants