The Contribution Mega-Thread

The advantage of gltf is pbr coloring. If you change to phong lighting, I feel that gltf is not needed. It is better to use the obj model. The mainstream is pbr coloring. I suggest that gltf should be pbr coloring by default. This is the advantage of gltf. If you really want to change to phong coloring, why not provide user settings, don’t write it in the code. Otherwise, it is painful to replace the materials one by one, especially when the scene is complicated. :worried:

Here’s another “Easy first fix” issue:

3 Likes

Here’s a not-so-easy fix for someone who understands terrain levels of detail:

1 Like

Another easy fix, I believe:

2 Likes

Pr sent for this
Edit: Which is closed until I fix it.

Hint: There is only one reason that:
this.material = material.clone();

…would throw an NPE… and it has nothing to do with what clone() returns. (clone() could return null all it wants and you will not get an NPE on that line)

2 Likes

a try clause?
Since we’re trying to catch the exception

No, we’re not trying to catch the exception.

1 Like

Never catch a NullPointerException in code you control. It looks dumb.

1 Like

Yes, ill remove that right away.

I’m going to put you out of your misery because this is taking far too long.

The clue is in the name of the issue. No material has been set. Material is null. You can’t call a method on a null object. You need to check if the material is null before you try to clone it, and if it is, you can’t clone it, because it’s null.

I commented on his PR with the line that needs to change:
this.material = material.clone()
becomes:
this.material = material == null ? null : material.clone();

Edit: and note that I haven’t looked at the context to see if we should really be calling JmeCloner to clone the material anyway… which I think would already handle the null.

Yes, and the PR does just that

Another “never” never ever ever ever throw an NPE yourself. There is no reason to do it.

2 Likes

Having material as a null is a valid state when cloning the TerrainPatch to put it in other words. It shouldn’t crash.

And IF it should crash, then the exception to give out would not be NPE, it would be controlled i.e. IllegalArgumentException. At least typically. Can’t really speak for the project.

This is just general, not related to the task:
Generally try-catch coding should be avoided wherever possible. Usually you can check the conditions before “trying” something out. For performance, design and readability reasons at least.

Yeah, it’s not clone()'s job to do validation. Its job is in the name.

I fixed it myself because it exceeded my ‘wasting time’ threshold 2-3 messages back.

Also added some comments because the cloning in this class is dodgy for other reasons.

And for clarity, try/catch causes no performance loss issue unless it’s thrown. If it is thrown, all bets are off.

1 Like

It’s a little more complicated than that:

…but yes, in general one should not avoid a try catch solely for performance reasons.

There are sometimes plenty of other good reasons to avoid them (like in this case) but mostly it’s because they are unneeded or there was a better way to structure the code in the first place (like try/catch around inputstream.close() for example).

5 Likes

Simple one line change: OGRE Importer should call saveInitialPose, to be consistent with GLTF · Issue #1395 · jMonkeyEngine/jmonkeyengine · GitHub

1 Like