JOML-CI / JOML

A Java math library for OpenGL rendering calculations
MIT License
727 stars 103 forks source link

GLM support #72

Closed elect86 closed 8 years ago

elect86 commented 8 years ago

Hi,

I am really looking forward to use joml in all my projects. Most of them are ports of C/C++ codes, where glm is intensively used.

Would it be possible to integrate in joml (some of) the glm functions?

To have an idea what I am talking about you can see this or this

httpdigest commented 8 years ago

Hi, fellow-Hamburger :) Sure. Please do a pull request containing the functions you need and how you envision the new methods/classes to look like. Then we could discuss based on that and align it with JOML's current design.

elect86 commented 8 years ago

hi dear ^^

So, I cloned joml and started to moving things inside modifying them in order to match your design. No extra allocation is performed.

But I am stuck because "static import declarations are not supported in -source 1.4"

I have to import Glm.mod289() inside Vector3f.

You can take a look here and here.

httpdigest commented 8 years ago

Thank you for your efforts!

But I am stuck because "static import declarations are not supported in -source 1.4"

You can perfectly get away without static imports. Just write Glm.mod289 then. I mean the dependency from Vector3f to the new Glm class is there anyway, isn't it? Static imports don't change anything there. They just make the source code harder to read if one wants to understand where that dependency is coming from. :)

On another note: I wouldn't call that utility class Glm btw. I would factor the relevant responsibilities out into separate classes. Two responsibilities I see currently: "Color conversion" and "Noise".

Also there is one unnecessary method Glm.floor(float) that just delegates to the JRE method. That should be removed. Another thing is: Do we really need those Glm methods operating on arbitrary float arrays, like Glm.step and Glm.subtr? And methods seemingly operating on a 4x4 matrix, in particular Glm.yawPitchRoll should also be removed. Matrix4f already supports that with the rotateXYZ/YXZ/ZYX methods.

elect86 commented 8 years ago

Forget it, it looks like I don't need that import anymore, probabily it was added when I pasted something.

Anyway, I will leave Glm for the moment, as a container for everything that doesn't have yet a precise destination. I created a ColorSpace class. What would you put inside Noise?

I know Glm.floor(float) is a jre method but I wanted to emulate glm.. anyway, removed.

The rest of Glm still need to be "elaborated".

httpdigest commented 8 years ago

What would you put inside Noise?

Simplex.

The rest of Glm still need to be "elaborated".

Okay. Tell me when you have something ready and do a pull request, so we can discuss based on it.

httpdigest commented 8 years ago

but I wanted to emulate glm..

That should not be the goal (at least not with JOML), in my opinion. The goal should be to provide all the functionality that GLM provides, not to do a port of GLM in JOML. The difference is that with the former way we allow to remove parts we don't want/need or have duplicates of, and we allow to reorganize the code (refactor method and class names and such) and also integrate other parts of JOML in it (for example using Matrix4f and Vector3f/4f where GLM originally only used plain float[] arrays).

This is because it could be that someone else will find another math library (let's call it "Libcmath") worth having in Java/JOML and then decide to port those functions into a Java class called CMath. There would obviously be intersections of common functionality between original JOML, GLM and CMath then.

So it would be better to extract the basic functionality and responsibilities that are needed out of those libraries in separate classes inside JOML which make their responsibilities obvious by name. But of course we can and should reference where we got/ported those functions from by adding a JavaDoc comment and say "this is that function from GLM/CMath."

elect86 commented 8 years ago

I know, I just wanted to point out why I wrote something apparently superfluous, anyway, as I said, I removed it.

However I finished, take a look. Noise.simplex() requires 26 temp variables and it looks crap , we have a couple of options, we may:

httpdigest commented 8 years ago

We also have the option to expand all vector operations and only have primitive floats. This was the way I avoided having temporary vector objects in every other JOML class. The simplex one is however the most complicated.

elect86 commented 8 years ago

Wouldn't this be slightly slower by a performance point of view?

httpdigest commented 8 years ago

Nah. It would even be slightly faster in the first few runs before being JIT'ted because of no method invocations. :)

elect86 commented 8 years ago

Ok, let me rewrite them so

elect86 commented 8 years ago

I started with ColorSpace but now I am stuck here.

The first saturation needs a tmp mat and vec4, because it uses the mat as a partial result got from the second saturation.

httpdigest commented 8 years ago

"inlining" is the solution. :) Just inline the saturation method below and get rid of the matrix object.

elect86 commented 8 years ago

I still have to finish, but I am afraid you won't accept my new changes since I am allocating new float arrays. Is this ok for you or not?

httpdigest commented 8 years ago

We might probably just want to look for a better original implementation of simplex noise, so as not having to manually inline and expand everything. Googling a bit for "Simplex noise Java" brought this up: http://staffwww.itn.liu.se/~stegu/simplexnoise/SimplexNoise.java We may use it and attribute to its original author (as mentioned in the JavaDocs). Having one-time statically initialized arrays there is fine, because they do not add any runtime overhead for successive invocations, nor are they being used as written shared memory, which would hinder multithreading. But even better would be to change those to instance fields and let the user instantiate a "Noise" class herself when needed and live with the per-instance allocation.

elect86 commented 8 years ago

Sincerely I already spent way too much time on this "simple" simplex, I'd prefer to not dig another class/method if possible (also because it seems that there are different types of this method).. Since this is a method I use only once in one example.. can I write it down using static vec variables like the example you googled and we postpone the discussion about the method when more serious and complex needings will raise up in the future?

httpdigest commented 8 years ago

I feel with you about your time. It's just that if there is a better solution and if we save future time integrating it into JOML because it already fulfills our requirements, then I'd go with that solution. But you can of course keep your implementation. I quickly added that mentioned SimplexNoise class into a new branch noise. Maybe you can use that. Would also be interesting whether and how much the results with that method differ from the GLM one and of course to benchmark them against each other.

pellepl commented 8 years ago

Just a heads-up if you aren't aware of it: Perlins simplex noise has a patent from 3rd dimension and up. I am not sure this patent applies for everything and haven't digged too deep into its implications. However, it seems people choose OpenSimplex noise instead for this reason. Apparently not as efficient as Simplex noise, but still is O(n^2).

httpdigest commented 8 years ago

This is not Perlin's version. It is Stefan Gustavson's version, which is patent-free.

elect86 commented 8 years ago

I totally agree, Kai, it'd be interesting profiling, benchmark and so on, but given I am using it only once in one sample (over almost 300 ones), as I said, all of this will require a lot of time, time that unfortunately at the moment I don't have (and I already abused somehow). So right now I have no other choise that to postpone it.

httpdigest commented 8 years ago

Yes, I'm sorry. I did not mean to imply any kind of commitment to any timeline/deadline with that statement.

elect86 commented 8 years ago

I gave a look to the class you proposed, I think there is no way I can get the same output of the glm one.

Even supposing that I get the same output, I don't get what is the difference between Gustavson's with static variables and glm's one modified to use static variables

httpdigest commented 8 years ago

Could you please test whether the SimplexNoise class suits you? Thanks!

elect86 commented 8 years ago

I don't need that anymore, thanks