I know that MikktspaceTangentGenerator is experimental (according to its javadoc), but it fails miserably with a NullPointerException if a geometry doesn’t have a normal map. I know that to calculate tangents, the geometry needs a normal map, but the same doesn’t happen with TangentBinormalGenerator.
Is that NPE supposed to happen? Or if a geometry has no normal map shouldn’t it just ignore it? The thing is I have a spatial with two geometries and only one of them has a normal map.
Math is not my strong point, but i think the tangents are calculated using mesh data only.
Yes this is my understanding as well.
Tangents are indeed necessary in the shader in order for Normal mapping to render correctly, but a normal map should not be required in order to generate the tangents. You should even be able to generate tangents on a Geometry without any materials, since tangents are stored in a mesh buffer and not the material.
I double checked the code in MikktspaceTangentGenerator and can confirm it does not appear to reference any normal maps or textures.
Could you post the full Exception that you are getting?
And also, are you getting this exception when generating tangents with code, or are you using the SDK (or any custom editor) to generate tangents? Or does the exception happen both ways?
I think OP mistypes and his meshes are missing a normal BUFFER… not a normal map.
…which a tangent generator would certainly need.
The issue here is one of error handling. (Personally, I come from the school of “NPEs should never be thrown” but then Oracle/Java-masters had to go poop all over that by having utility code throw NPEs instead of IllegalArgumentException… but I digress.)
The question here: If you give a tangent generator a subgraph with a variety of meshes… should it choke and fail on meshes without normal BUFFERS or just silently skip them… or something in between?
I think that’s the actual issue. This is the exception:
java.lang.NullPointerException: Cannot invoke "com.jme3.scene.VertexBuffer.getData()" because "normal" is null
at com.jme3.util.mikktspace.MikkTSpaceImpl.getNormal(MikkTSpaceImpl.java:86)
at com.jme3.util.mikktspace.MikktspaceTangentGenerator.getNormal(MikktspaceTangentGenerator.java:755)
at com.jme3.util.mikktspace.MikktspaceTangentGenerator.MergeVertsFast(MikktspaceTangentGenerator.java:460)
at com.jme3.util.mikktspace.MikktspaceTangentGenerator.generateSharedVerticesIndexList(MikktspaceTangentGenerator.java:412)
at com.jme3.util.mikktspace.MikktspaceTangentGenerator.genTangSpace(MikktspaceTangentGenerator.java:190)
at com.jme3.util.mikktspace.MikktspaceTangentGenerator.genTangSpaceDefault(MikktspaceTangentGenerator.java:152)
at com.jme3.util.mikktspace.MikktspaceTangentGenerator.generate(MikktspaceTangentGenerator.java:143)
at com.jme3.util.mikktspace.MikktspaceTangentGenerator.generate(MikktspaceTangentGenerator.java:113)
at org.fba.jme3.MikktspaceTangentTest.simpleInitApp(Unknown Source)
at com.jme3.app.SimpleApplication.initialize(SimpleApplication.java:240)
at com.jme3.system.lwjgl.LwjglWindow.initInThread(LwjglWindow.java:608)
at com.jme3.system.lwjgl.LwjglWindow.run(LwjglWindow.java:712)
at java.base/java.lang.Thread.run(Thread.java:1575)
Without getting too philosophical, I guess it should do its best to operate the same way as the regular tangent generator… which probably does not throw an NPE in this case.
Here is a simple test case. Note that it works if TangentBinormalGenerator is used.
package org.fba.jme3;
import com.jme3.app.Application;
import com.jme3.app.SimpleApplication;
import com.jme3.effect.ParticleEmitter;
import com.jme3.effect.ParticleMesh;
import com.jme3.light.DirectionalLight;
import com.jme3.light.Light;
import com.jme3.material.Material;
import com.jme3.material.RenderState;
import com.jme3.math.ColorRGBA;
import com.jme3.math.Vector3f;
import com.jme3.scene.Geometry;
import com.jme3.scene.Node;
import com.jme3.scene.shape.Box;
import com.jme3.util.TangentBinormalGenerator;
import com.jme3.util.mikktspace.MikktspaceTangentGenerator;
public class MikktspaceTangentTest extends SimpleApplication {
@Override
public void simpleInitApp() {
final Light light = new DirectionalLight(new Vector3f(.5f, -.5f, -.5f), ColorRGBA.White);
rootNode.addLight(light);
final Material material = new Material(assetManager, "Common/MatDefs/Light/Lighting.j3md");
material.setBoolean("UseMaterialColors", true);
material.setColor("Diffuse", ColorRGBA.Green);
final Geometry geometry = new Geometry("Box", new Box(.5f, .5f, .5f));
geometry.setMaterial(material);
final Material particles = new Material(assetManager, "Common/MatDefs/Misc/Particle.j3md");
particles.setTexture("Texture", assetManager.loadTexture("Effects/Explosion/flame.png"));
particles.getAdditionalRenderState().setBlendMode(RenderState.BlendMode.AlphaAdditive);
final ParticleEmitter emitter = new ParticleEmitter("Particles", ParticleMesh.Type.Triangle, 1000);
emitter.setMaterial(particles);
emitter.setStartColor(ColorRGBA.Yellow);
emitter.setStartColor(ColorRGBA.Orange);
emitter.setLowLife(.5f);
emitter.setHighLife(1.2f);
emitter.setImagesX(2);
emitter.setImagesY(2);
emitter.getParticleInfluencer().setInitialVelocity(new Vector3f(1, 2, 0));
emitter.getParticleInfluencer().setVelocityVariation(.2f);
final Node node = new Node();
node.attachChild(geometry);
node.attachChild(emitter);
MikktspaceTangentGenerator.generate(node);
// TangentBinormalGenerator.generate(node);
rootNode.attachChild(node);
}
public static void main(String... args) {
final Application app = new MikktspaceTangentTest();
app.start();
}
}
It looks like TangentBinormalGenerator handles missing normals with a simple check for a normal buffer (and texCoord buffer) before it attempts to generate the tangents:
So it should be an easy fix to add this if-check into the generate() method of MikktspaceTangentGenerator :
if (mesh.getBuffer(Type.TexCoord) != null && mesh.getBuffer(Type.Normal) != null) {
//proceed with tangent generation for mesh
}
If anyone else wants to submit a PR, feel free to do so. Or if no one gets around to it first, I’ll create a PR in the next few days.
Another thing to mention, although slightly unrelated:
I’d also like to try to change this for the next 3.8.0 release. It appears that MikktspaceTangentGenerator was created and labeled as experimental back in 2016 by nehon, and Riccardo currently has an open PR to deprecate TangentBinormalGenerator : Deprecate TangentBinormalGenerator by riccardobl · Pull Request #2144 · jMonkeyEngine/jmonkeyengine · GitHub
So once this issue is resolved, I think it is finally time to deprecate the old and move forward making MikktspaceTangentGenerator the default, and then try to get as many jme users as possible to test it during the alpha and beta phases of the upcoming 3.8 release. And if no further issues are found, it can make it into the stable release.
Since i was scrolling through the code too, the MikktspaceGenerator contains quite a few of todo’s. one of the comments seems to indicate that the original implementation uses some pointer magic.
So the questions standing is, is the jme mikktspace generator tested against other implementations? since the primary reason for it’s existence is to have an industry standard that should produce the same results in every engine/normal map backer
It likely is not, but I assume the old TangentBinormalGenerator hasn’t been tested to match industry standards for pbr either, since it came before jme got pbr and started aiming to match the khronos/gltf and similar industry standards.
I also don’t think this is a feasible thing to test. I assume the only way to test jme against other implementations would be to render a model with a normal map in JME and compare that to the output rendered in the krhonos viewer under exact same lighting conditions.
But there’s already some other notable differences in JME’s final render output for pbr when compared to the industry pbr standard (one that comes to mind is the recent fZero issue you opened that is currently one of a few possible issues that is causing our pbr shader to not match industry standard. And who knows what else is wrong in our implementation, especially since the author left soon after creating it, I expect the bug squashing process for the pbr shaders will be an ongoing process for a while). I assume any differences in a jme implementation will be very subtle, so you couldn’t really test just the MikktspaceTangentGenerator unless you are already certain that every other aspect of jme’s pbr implementation is already correct.
Edit: I just realized that you could test jme’s tangents if you find a model that had its tangents generated with a different MikktspaceTangentGenerator from a source that we know is correct and then compare the tangent values with code, and not have to base it on visual output. But that still is a non trivial task that could be addressed in a future dedicated pr, and I’m not certain that is worth holding up implementing jme’s MikktspaceTangentGenerator in place of TangentBinormalGenerator considering no major rendering issues have been reported with jme’s implementation, and the current implementation appears to atleast work better than the old TangentBinormalGenerator it would be replacing.
But either way, MikktspaceTangentGenerator appears to already be the suggested one to use in the documentation, and I think the SDK already uses it instead of TangentBinormalGenerator too. The PR I linked from riccardo mostly just replaces TangentBinormalGenerator in some test cases, so it is mostly just a final effort to phase it out everywhere in the engine so no one accidentally uses it. But I agree it is still important to make sure that our new implementation is correct.
http://www.mikktspace.com/ states that blender > 2.57 uses it. So we could test against a exported gltf. (assuming the gltf loader uses the exported values)
I am not against making mikktspace the default generator in jme, my fear is that i we add another not tested variable in the already possible slightly off pbr environment it gets harder and harder to track down the issues.
That said, i really have no clue how many users of the pbr renderer exists, so i cannot say how tested the codebase is. Considering the monthly screenshot threads, my guess is that not many are using pbr actually.
Maybe it could be a good idea if I open an issue to document the fact that our PBR implementation is suspected to have a few minor discrepancies from the khronos standards, and use that to keep track of potential things like this that may be incorrect.
It could be a while until the PBR shader renders perfectly, especially considering the fact that no one has really delved deeply into the math behind it since nehon made it (as far as I’m aware). But at least if we keep track of this all in an issue then that should help narrow things down in the long run.
True, it is unfortunate it doesn’t get more use. I think many people avoid PBR because it seems like more work than the old Lighting.j3md shader.
But I personally have come to find PBR to actually be easier to make finished scenes with, since it is easier to blend different art styles from different artists together when you have more control over the lighting with a lightprobe. After switching to PBR, many of my scenes became much more cohesive and I felt like I struggled less trying to find models that fit together.
Regarding to this topic, what is the usecase for having meshes with and without normals mixed. Imho it is clearly an error if you want to generate tangets on a mesh without normals. Current implementation seems more to follow a “generate tangets on the whole scenegraph” approach
In a particular subgraph, you could have a bunch of meshes with different materials for a variety of reasons, I guess.
If I pass in a subgraph with 49 normal-having meshes and 1 non-normal having mesh, should it throw an NPE? What would be the good reason for making the user split out all of their non-normal having meshes, run the generator, and then recombine them?
If I pass in a subgraph with 50 non-normal having meshes, should it pass silently and do nothing?
It’s also hard to say because whether having or not having tangents is an error or not depends on the material in the end. (In the early days, I often wished it was an error when a material that tried to use tangents didn’t find any.)
Anyway, I suppose it’s quite common to throw a whole loaded model into the generator “just in case”… regardless of whether some of those children are lighting, or PBR, or unshaded, or particles, or whatever.
Yeah, if that is a valid usecase then the old behaviour is correct.
I’ve opened a PR to fix the null pointer exception, and handled it the same way that TangentBinormalGenerator does.
The change is very minor, only 2 lines were added and nothing else was changed (even though github’s comparison says otherwise). Any review is appreciated.
…that’s because the indent of that entire section was changed because it’s now in an if block. Possible to avoid by inverting the condition and doing an early return but not really worth it… and in this case I’m on the fence about whether that makes the code more confusing or not. (I generally prefer a good early return over deep nesting but I’d also have structured this whole file a bit differently anyway. Style is as style does.)
This issue should now be fixed in the latest 3.8.0-alpha2 release.