openfl / lime

A foundational Haxe framework for cross-platform development
https://lime.openfl.org/
MIT License
749 stars 359 forks source link

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

Closed UncertainProd closed 1 month ago

UncertainProd commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

Cool

joshtynjala commented 1 month ago

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 commented 1 month ago

I'll take a look.

player-03 commented 1 month ago

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?

https://github.com/openfl/lime/blob/1b8d7ac7fa39aaabae51136ff0bcd9f75b98879e/project/src/graphics/cairo/CairoBindings.cpp#L805

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.

https://github.com/openfl/lime/blob/78e99bf1d92266a06605e32147c9c6ba3a6a03dc/project/src/math/Matrix3.cpp#L70

joshtynjala commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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

player-03 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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

nanjizal commented 1 month ago

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 ).