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

Flash.geom implementation interpolate, interpolateTo, pointAt, pointTowards #18495

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

Conversation

emergence75
Copy link

this is my first time opening a pull request...

i did some cleanup for different variable names and added the docblock information needed for debugging, rebuilding this methods mainly to understand how this could/should work...

here is an implementation for Matrix3D.as

static function interpolate
function interpolateTo

function pointAt (possible implementation)

extended version for re- and decompose including skew and perspective handling
there is an addition static method correct included to allow interpolation for scale,skew and perspective (per default false)
false == like flash does interpolation

the recompose method also includes a quaternion error message when the given vector is not correct...

and an additional switch
static function correct
if set to true interpolate will correctly include scale, skew and perspective

there is a helper function included function helperCustomRound, mainly for debugging reasons...

there is also an implementation for Utils3D.as

function pointTowards

i've played arround a little bit and the result is pretty nice... might be useful

also added PerspectiveProjection.as missing methods...
not really tested, but should be working...

some additional notes when playing with interpolate
the implementation in flash is wrong... and its a bad idea to mimic the behaviour for interpolation (mainly when using scale)
the quaternion calculation is not correct, which leads to slight differences in rendering...
for more information take a look here (testfiles also included there...)
airsdk/Adobe-Runtime-Support#3506

implement

interpolate
interpolateTo
pointAt

extended version de- and recompose

additional static function correct per default false...
if set to true scale,skew and perspective are also interpolated..

removed stub for recompose quaternion, throws error when quaternion is erroneous

added docblocks and some variable cleanup...
implement missing methods...

additional method to set znear and zfar added...
added method

pointTowards

added docblock
@danielhjacobs danielhjacobs added A-avm2 Area: AVM2 (ActionScript 3) T-fix Type: Bug fix (in something that's supposed to work already) T-compat Type: Compatibility with Flash Player and removed T-fix Type: Bug fix (in something that's supposed to work already) labels Nov 6, 2024
i'm getting old... removed last arg correct
@adrian17
Copy link
Collaborator

adrian17 commented Nov 6, 2024

I didn't dig deeply into this yet, not sure if evilpie knows more about this area but here's some of my quick observations:

  • added the docblock information needed for debugging - we don't copy AS3 docs to our source. Ideally can you try to keep the diff as minimal as possible?
  • Some of the changes lack rationale, for example removing if (value != null) { in Matrix3D.rawData setter almost certainly doesn't look right, same with value.AS::concat().
  • the implementation in flash is wrong - I'm assuming this is what the static var _correct is for? We're interested in exactly replicating what Flash does - and that includes bugs, unfortunately. We also don't want to add new API surface to FP classes.

Some of the implementations changed a lot - can you clarify which ones are "this is a more accurate representation of what Flash does", and which ones are "this is my implementation, more accurate than Flash's"?

@adrian17
Copy link
Collaborator

adrian17 commented Nov 6, 2024

also added PerspectiveProjection.as missing methods...
not really tested, but should be working...

Which missing methods?
"Not tested", as in tests not existing, or you haven't tried running them?

Without seeing example test cases where currently Ruffle shows incorrect results but the updated implementations are correct, it's hard to understand many of the changes.

I also don't see public function setZPlanes existing in Flash, or in newer AIR versions. Where can I read about it?

@Dinnerbone
Copy link
Contributor

Hey, welcome to the project and thanks for opening your first PR with us! We really appreciate it, and please don't feel discouraged by the review notes so far. It's all a process, and we just want things to be right :D

I haven't gone over this fully so far but I have a few initial notes of feedback + questions:

  1. Can you please provide tests for these changes? By skimming the code I can see a lot of potential edgecases that you try to handle - I'd like each one to be proven with a test please, to ensure we keep compatibility and avoid any future accidental regressions
  2. There's a lot of debugging code all over the place, we probably don't want that
  3. I had a look at the lookAt method, and I presume a lot of this is edgecase handling. Like adding back skew for example, is this something you've tested and confirmed? I figure lookAt would be a fairly trivial thing but I guess Flash has lots of corner cases?

correct AS::  => AS3::
@emergence75
Copy link
Author

i don't feel discouraged.

this pull request is meant to be more a draft. my intension is not that you take the pull request 1:1 to ruffle...
and you are right, some debug code like helperCustomRound isn't needed and can savely be removed...

it needs someone to dive in and adapt the useful code parts, if needed...
without the docblock sections its even harder to understand what matrix3D methods should do or better how they should work...
the main reason for me to play arround with this topic was

You don't need to understand matrix mathematics to use the Matrix3D class

but you need to understand it, to implement something in this area...

i wanted to understand how matrix3d operations work in 3d space and playing arround with complex numbers makes no fun. so i decided to play arround in flash visually with it...

i will provide more information for each section soon...

added something i missed from last checkin
@emergence75
Copy link
Author

simple changes i made in the Matrix3D.as

moved the

import flash.geom.Orientation3D;

to head of package...

removed function checkOrientation(orientationStyle:String)

and integrated it, in function decompose and recompose...

renaming some variables e.g in public function appendRotation
var d = m.rawData;
in
var mr = m.rawData;

e.g
public function get determinant()
mr = this.rawData;
and using mr to calculate the determinant...

public function transpose()
var oRawData = this._rawData.AS3::concat();
in
var mr = this.rawData;

main reason, better readability...

@emergence75
Copy link
Author

emergence75 commented Nov 8, 2024

lets start with
public function decompose

per default flash does not take care for skew or perspective information which is embedded in the matrix
the changes here extract this information and append it the returning array...

this wont break any compatibilty...

public function recompose
works the same way as currently implemented in flash, except it can handle additional skew and perspective information...

the logic to apply scale direct on the rawData has been moved to the end of the method, so that skew and perspective can correctly be applied...

from what i tested so far, this does not break any compatibilty

additionally there is a possible check included when method is quaternion, if the given vector is really valid...

both methods are now more generally useable and more correct...

@emergence75
Copy link
Author

public static function get correct()
public static function set correct()

per default the setting for ruffle is set to false (== do it the flash way, don't interpolate scale,skew perspective)

when interpolating and the scale for both matrix is set to 1,1,1
the result is identically to how flash does it...

when any matrix has a scale, flash does something that results in a litte different quaternion...
more information about this see
airsdk/Adobe-Runtime-Support#3506

public function interpolateTo

logic is different to method interpolate, percent 0 means we start at the toMat

public static function interpolate

the method uses the method decompose to get all components for each matrix
and does the interpolation like descriped here
https://www.w3.org/TR/css-transforms-2/#matrix-interpolation
i found this information when i allready had rebuild this method...

after interpolating each part, internal method recompose is used to build the new matrix

a demo how this looks like in flash can be found here
airsdk/Adobe-Runtime-Support#3506

static method correct can be used to force a correct behaviour for interpolate...

@emergence75
Copy link
Author

emergence75 commented Nov 8, 2024

public function pointAt

this is a prototype how it could work... the fun part here is that flash does not remove scale, skew or perspective
it seams it does it the correct way...

Utils3D.as
public static function pointTowards

also a prototyp using Matrix3d.pointAt and Matrix3D.interpolate
flash removes here scale, skew, perspective

mainly a test when playing arround with interpolate...

the different behaviour removing scale, leaving scale was the reason to implement the method function correct

PerspectiveProjection.as

a possible implementation how it could work...
i had no time to test the class... thats also the reason why public function setZPlanes exists...
so when testing you can manipulate these values without recompiling ruffle...

the next part if i find some time, i wanted to render this whole logic in ruffle...
so far i have not found any code which does something like this
https://skannai.medium.com/projecting-3d-points-into-a-2d-screen-58db65609f24

i'm pretty sure that for this we need a correct and complete decompose / recompose logic (the reason why i've extended these two methods)

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