Had little time today to implement and test everything.
But so far those are a lot of additions and even one small fix. 
I will be pushing the changes tomorrow evening (about 22:00 CEST).
So far, creating an euler rotation matrix is waaaay faster compared to libGDX. Handcrafted and optimized and tested that.
(Not going through a quaternion was a good choice of yours to want that. )
And it is kinda intelligibly (IMHO) named in both the matrix and quaternion classes: rotationXYZ/rotationZYX and rotateXYZ/rotateZYX, which do equivalent things in both kinds of classes and are always equivalent to the call sequence: [icode]rotateX/rotationX(…).rotateY(…).rotateZ(…)[/icode] and [icode]rotateZ/rotationZ(…).rotateY(…).rotateX(…)[/icode], although of course alot faster.
We’re having problems with Euler angles in WSW. It seems like the order we’re supposed to use depends on if you use a Y-up or Z-up coordinate system. We’re investigating how to solve it (it was basically hacked before), but if possible it’d be great if you could implement all 6 orders (XYZ, XZY, etc) so we can try them out. I realize that that might be a bit tedious, but I think it will be useful for people with odd coordinate systems. If you think it’s a bad idea, I can give you our specific requirements when we figure them out.
Yes, please figure this out first. I pushed a hotfix for Matrix.rotationZ, and now you can always use the expanded form to figure out your rotation order:
[icode]rotationX(…).rotateY(…).rotateZ(…)[/icode] (first must be ‘rotation’ instead of ‘rotate’) with any order to create a rotation matrix from euler angles.
What are you using euler angles for? IMHO the only sane place is certain styles of 3rd person camera control (and maybe avionics). Where do you think are they helping vs. hurting you?
For the library…one trick here is that are are (if memory serves) 26 unique euler angle like systems.
I’ve found a bug. Quaternion*.set(Matrix**) does not work correctly with scaled matrices:
Quaternionf q = new Quaternionf();
Matrix4f m = new Matrix4f();
for(int i = 1; i <= 20; i++){
float scale = i / 10f;
q.set(m.identity().rotate(45, 0, 0, 1).scale(scale));
System.out.println("Scale " + scale + ": " + q.x + ", " + q.y + ", " + q.z + ", " + q.w);
}
Prints:
If it does not work for scaled matrices, it is pretty much a useless feature IMO. Example: I want to find the world orientation of a bone. Convert object matrix rotation to quaternion, multiply together bone rotation with model rotation. Currently this fails if the model is scaled, which it pretty much always is.
EDIT: This is the Euler angle rotations we need:
quaternion.identity().rotateY(yaw).rotateX(pitch).rotateZ(roll);
This is most likely due to us using Y-up instead of Z-up.
Thanks for reporting! That could be interpreted as a bug in the method contract specification in the JavaDocs.
That method only supports orthogonal matrices (those with orthonormal column vectors) as input.
I renamed that method to “setFromNormalized” and added a “setFromUnnormalized” that supports any matrix with possible scaling.
That new method uses the same approach as libGDX by first normalizing the column vectors.
It’s in now. Use [icode]Quaternion.rotationYXZ(…)[/icode] or [icode]Matrix.rotationYXZ(…)[/icode].
How would you feel about me doing a series of “Golden Axe of Refactorization” push requests? With an explanation of each?
[quote=“Roquen,post:327,topic:53459”]
To anyone thinking about contributing to an open source project:
Just do it, you don’t have to ask.
You’ll feel better and the project owner will feel better. A pull request is a clean, fast and simple way to contribute. Even if it gets rejected, the discussion under the PR will be useful. You don’t have to commit too much time either. Even if you’ve got big ideas, it’s generally best to submit contributions in small batches.
Yeah I’m a firm believer in “It’s better to ask forgiveness than permission”. What I had in mind was removing trivially inlinable (aka hotspot doesn’t even think about it…just does it) code repeats into methods. So it would be probably hundreds of small changes which produce identical native code.
I highly agree with @Spasi.
Providing something (i.e. code) to discuss about is always better than asking “maybe, should I do something?,” where no one knew the answer to that question because there is no basis on which to discuss.
By the way: It might have disappeared under all the other discussions, but you might remember that everytime someone wanted to have something in JOML I always asked for a pull request.
And if the discussion went along and detailed more and more what it concretely was that the person wanted to have and why he/she wanted to have that, I opted in and implemented it myself once I undestood.
[quote=“Roquen,post:329,topic:53459”]
That’s OK. The useful metric isn’t “how many line changes in the PR”, but “how easy it is for the project owner to review the PR”.
Yes, please do this. BUT keep in mind the reason of refactoring: to make future code changes easy. There is no point in refactoring methods that will never change again in JOML, such as maybe refactoring the copy constructor of Vector4f and its respective set() method into a single common private method. This would be a code change that may reduce three lines of code, but will have no future benefit. Also that kind of refactoring would reduce readability of the code, when one has to follow method invocations to see what the code actually does.
So, please spare yourself the effort of trying to refactor trivial methods. Complex methods however, that are likely to change, because people might find faster ways of achieving what that method does, are good candidates for refactorings.
So I started a re-factor but stopped (or paused). There are too many issues of interest before putting the effort in. First: there are tons of redundant functionality for marginal (approaching useless) usage (as I’ve mentioned before) and conversely large holes in common functionality.
But the other big issues is the lack of contracts. Users are given too much freedom in value ranges which serves zero purpose and this leads to much more complex code (runtime cost). Users should be limited to partially sane values and the library could provide conicalization methods for users than want to pass in completely random values.
I am a big fan of “improve it in small steps,” while those steps should also always add value to a library and allow it to evolve. So, please tell me concretely one or two functions that you want to have added to fill the “large holes in common functionality.”
Then we can change things.
Also, please name one or two concrete functions whose contracts should be altered and their implementations then simplified. I don’t like to say that again, but again you are being way too vague to induce any change.
So, make things concrete.
Then, we can change things.
The big question is: Is it more important to allow user to supply random input? Or place logical restrictions on what’s deemed legal and if the user breaks the contract: garbage-in, garbage-out? This change everything.
I see your point, but can you please make a concrete example with concrete (sane) value ranges for a concrete method in a concrete class?
You see, proposing to “change everything” is just not in there anymore, as there are some number of people now relying on the existing semantics of JOML’s methods, and changing everything will break them.
We have to adhere to the Open/Closed Principle here: Change JOML not by modifying it, but by extending it.
There was such a point earlier, when @theagentd requested there to be a matrix multiplication method which assumes the last row to be (0, 0, 0, 1), as that was a typical use case for him. So, we added a new method mul4x3 to JOML. No big deal: No modification of existing code, but extension by adding new code.
That’s how things have to be now.
Yet another disclaimer: This is my kinda thinking and the intention of my comments is improve the library, of which I will never be a user because I never use basic math libraries.
Backward compatibly is: proxy method and refactor OR these classes aren’t final…app extents class with marginal use-cases they desire in place. Forever-and-ever library backward compatibly is a different design space. Besides nobody has to change from their current version.
On sane ranges examples. The axis angle classes and conversion thereof should assume the axis is unit magnitude and that the angle is on [-pi, pi]. Likewise on raw input.
Note that AxisAngle4f is used in about 190 methods. About 2 of these perform any work. Direct transform of a vec3 and vec4. Two forwarding methods to the xform. The remainder is due to a combinatorial explosion of type conversions.
This is pretty wide spread and serves little to no purpose. Some examples:
Mixing and matching singles/doubles: [icode]public Vector2d add(Vector2f v, Vector2d dest)[/icode]
oh I think you get my attempted point.
It does serve purpose, since it was being requested by a user. What is your definition of “serving a purpose?”
What do you think about my main point of sensible input ranges and an aux method to enforce at user’s request?
You’re focusing on one element of a combinatorial set. I suggested way up near the top to let the compiler (and specifically inlining) do the work for you. Nobody needs to read/write and maintain.
Tossed all my old refactor…starting a new one that’s less ambitious. As a data point for my argument: public AxisAngle4f set(AxisAngle4f a). Pointless code repetition leads to bugs. QED.