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

Make Matrix3 an abstract over Float32Array (just like Matrix4) #1778

Merged
merged 3 commits into from
May 27, 2024

Conversation

UncertainProd
Copy link
Contributor

lime.math.Matrix4 is an abstract over Float32Array which makes it possible to pass directly into gl functions like GL.uniformMatrix4fv but this does not seem to work for Matrix3 and GL.uniformMatrix3fv unless we convert it into a Float32Array beforehand. So making Matrix3 an abstract as well should make it behave just like Matrix4

@UncertainProd
Copy link
Contributor Author

Btw I checked if Openfl ever extends Matrix3 for any reason, but I did not find any such instance, so this probably won't be a breaking change, since the public facing functions and fields are all the same as before.

Also, should I make this PR to the develop branch or the 8.2.0-Dev branch?

@player-03
Copy link
Contributor

develop is for bug fixes, 8.2.0 is for new features, 9.0.0 is for breaking changes. It can be hard to tell sometimes, but I'd lean towards "new feature" here.

@UncertainProd
Copy link
Contributor Author

Cool

@UncertainProd UncertainProd changed the base branch from develop to 8.2.0-Dev April 20, 2024 08:41
@player-03 player-03 merged commit d0b1f6b into openfl:8.2.0-Dev May 27, 2024
26 checks passed
@UncertainProd UncertainProd deleted the matrix-3-abstract branch May 27, 2024 21:19
@joshtynjala
Copy link
Member

Uh-oh! I think that this change has broken some CFFI functions for Cairo when targeting HashLink.

I noticed this call in particular is failing (because it's now getting a different type than it expects), but I think that there are others that are affected too.

// in lime.graphics.cairo.Cairo get_matrix()
NativeCFFI.lime_cairo_get_matrix(handle, new Matrix3())

@player-03
Copy link
Contributor

I'll take a look.

@player-03
Copy link
Contributor

player-03 commented May 29, 2024

First impression: I don't think I like how CFFI handles Matrix3. Too many shortcuts and assumptions.

This code assumes that a is the first variable and that the variables are tightly packed. I'm sure it's true at the moment, but what guarantee is there that hxcpp won't change?

cairo_get_matrix ((cairo_t*)handle->ptr, (cairo_matrix_t*)&out->a);

This code assumes that the class's second constructor has already initialized id_a and so on. I assume the current code always does things in that order, but if we ever wrote a new function, it would be easy enough to call the wrong constructor.

alloc_field (matrix3, id_a, alloc_float (a));

@joshtynjala
Copy link
Member

Okay, with #1791 being merged, HashLink is working. However, I'm seeing that gradient fills aren't rendering in OpenFL with neko/cpp. The gradients seem to be rendering correctly on HashLink, though.

One thing I see that makes me suspicious is that the documentation says the layout of the matrix is like this:

[ a, c, tx ]
[ b, d, ty ]
[ 0, 0,  1 ]

However, the constructor has this layout instead:

[
	a, c, 0,
	b, d, 0,
	tx, ty, 1
]

@player-03
Copy link
Contributor

The documentation in the rest of the file isn't any more consistent. Some of them show tx and ty on the right, some on the bottom. Some show the first four as abcd, others as acbd.

But also, I don't think existing code should have been affected by this change. Yes it arranges the values in an array, but the existing code only refers to the variables, mapping tx to index 6 and ty to index 7. Even if those are the wrong indices, it should still find them there.

I'll pull up the Wikipedia article and take a closer look.

@nanjizal
Copy link

nanjizal commented May 30, 2024

hi from memory flash was never very good at matrix. My suggestion is to just create a new Matrix3 and Matrix4 and leave the old without changing. Then users can upgrade if needed.
For webgl actually a 4x3 matrix is sometimes optimal, you kind of cheat with multiplication, but it saves on calculations.
The 4x3 approach came from this project.
http://tulrich.com/geekstuff/canvas/perspective.html
and my port which had some restructuring and author agreed on mit license, I think he was impressed by my restructuring and felt it was almost a different project, as I was impressed with his code/maths ideas often on the edge or beyond me.
https://github.com/nanjizal/canvasImageTriangle/
I later used an abstract approach over structInit and implemented in my geom lib, 4x3 works well on webgl and gluon ( opengl ).
You can see tx is traditionally on the horizontal row ( for xyw or xyzw ).
https://github.com/nanjizal/geom/blob/master/src/geom/matrix/Matrix4x3.hx#L648
The author of hxMath is more mathimatical but my implementation was later and so my approach may have advantages ( I think that the abstract approach I took was better and more compiler optimal ) but check against his lib for correctness, certainly I took inspiration from many places including kha math and some c libs and my exploration of triangulation etc... Feel free to plunder and take the useful parts of my lib if helpful. But really believe should forget the flash matrix class and provide an upgrade solution that users can choose, perhaps that is true of a few parts of the flash framework, openfl should evolve and remove mistakes of openfl but keep good parts. I did study Linear Algebra at Uni and still have the books but my memory is lacking, but certainly have some matrix understanding and I would never choose flash matrix over hxMath or my geom lib.

@nanjizal
Copy link

for 3d projection https://github.com/nanjizal/geom/blob/master/src/geom/matrix/Projection.hx see my dice project.

@player-03
Copy link
Contributor

tx is traditionally on the horizontal row

I assume you mean "the first row"? Because the code you linked to shows it in the rightmost column.

Per Wikipedia, there are people who use both notations, but putting them in the right column is more common. (It's how I was taught, certainly.)

from memory flash was never very good at matrix. My suggestion is to just create a new Matrix3 and Matrix4 and leave the old without changing.

Lime's goal isn't to emulate Flash or to provide a full suite of math-related classes. As long as we have the Matrix3 and Matrix4 classes, it's probably worth making them as useful as possible, but I don't know that we need to expand into classes that Lime won't use.

joshtynjala added a commit that referenced this pull request May 31, 2024
…) (references #1778) (references #1792)

Fixes gradient drawing in OpenFL
@joshtynjala
Copy link
Member

However, I'm seeing that gradient fills aren't rendering in OpenFL with neko/cpp.

Figured it out. CairoPattern wasn't converting to the new CairoMatrix3. Gradients now rendering correctly with my fix.

@player-03
Copy link
Contributor

Whoops, guess I should have done a full search. Good catch.

@nanjizal
Copy link

far right is what I was explaining as per the link. Typical use is constraints using 1x4 and matrix/dual quaternion 4x3 https://nanjizal.github.io/try_geom/
https://nanjizal.github.io/dice/binWebGL/index.html ( geom/move/Axis3 and see dice repo ).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants