The normal parameter is not fully checked.

[drzhonghao]

When I call setNormal method of com.jme3.math.Plane, I notice that the method throws exceptions for null inputs: public void setNormal(Vector3f normal) { if (normal == null) { throw new IllegalArgumentException("normal cannot be null"); } this.normal.set(normal); }

However, other methods do not check similar parameters. For example, I found that setOriginNormal does not throw exceptions when normal is set to null:

public void setOriginNormal(Vector3f origin, Vector3f normal){ this.normal.set(normal); this.constant = normal.x * origin.x + normal.y * origin.y + normal.z * origin.z; }

Please also add checks to avoid ill usages.

[stephengold]

setOriginNormal doesn't throw any exception at all? Not even a NullPointerException?

I believe a NullPointerException is the correct outcome for these situations.

[MeFisto94]

1. Unfortunately jME is not consistent in it's code with error checking (at least that's my belief), this starts with several errors only being triggered with assertions on, others being unit tested, others not being tested at all etc pp.

2. For a game engine you probably don't want to check everything (things like setNormal might be called a thousand times). Or you only want them to be checked in the Debug Build. It's a matter of performance and there are way less obvious faults than null values (e.g. you'd have to check every normal's length or force-normalize it)

3. SetOriginNormal throws a NPE in the second line, at normal.x. Still it's users obligation to code correct code :P Maybe something like @NonNull could've helped with compile time checking there.

[drzhonghao]

I believe that the two methods shall throw the same exception for the same problem. Or else, it becomes inconsistent, and sometimes difficult to understand.

[stephengold]

For the sake of consistency, I'm willing to remove the IllegalArgumentException from from the setNormal() method.

[pspeed42]

I don't think it's difficult to understand either way. I mean, in one case you get an NPE that's cryptic and you have to track it down. In the other you get a nice message... but they are far enough away from each other that it's not like most people will think "Odd, the NPE can't be because of the argument I passed because it didn't throw an illegal argument exception..." If it does then maybe you are using the wrong library. ;)

Sometimes an NPE is much worse because it happens far from where the original error is... and almost always requires digging into the source code to find out what you did wrong. (Good coding practice is that any library code that throws an NPE is probably buggy.) I understand the arguments against a lot of checks everywhere but the null check is a pretty trivial one, too.

[Nehon]

To me using IllegalArgumentException in these case is the right thing to do. I've seen other library throwing NPE when arguments are null I I find it completely dumb. Even in Java core there is this kind of stuff. To me an NPE is the last ressort exception, meaning that's the one you got when the error was not anticipated at all. An IllegalArgumentException seems the perfect fit when you fed a method with an illegal argument.

So IMO here wee should add this check in the method where it's missing.

[stephengold]

Based on the lack of consensus, I've labelled this as "wontfix".

[pspeed42]

Hmmm... not sure there is a lack of consensus. Remy and I both believe that IllegalArgumentException is better than NPE-go-hunt-in-the-source-but-which-version-again... So if someone wants to submit a patch along those lines then it would be appreciated.

[empirephoenix]

I personally think that this is the case for the assert keyword, as it only needs to be enabled in development, and has no performance cost at deployed runtime

[pspeed42]

Could be. I guess many of us don't run with asserts enabled because of other issues. Is it still the case that JME will fail sometimes with invalid asserts?

[empirephoenix]

I run my projects with enabled asserts for quite some months, not sure if my machine was ever affected however

[Nehon]

Yes asserts for sanity checks could be nice. I remember having opengl errors failing with assertions, but not actually hindering the render...

[MeFisto94]

I think the "issues" with assertions where: When an openGL Error occurs (there used to be a GL_INVALID_OPERATION on AMD Cards), it is usually ignored/discarded, but with assertions on, it terminates jme when an GL Error exists.