jMonkeyEngine / jmonkeyengine

A complete 3-D game development suite written in Java.
http://jmonkeyengine.org
BSD 3-Clause "New" or "Revised" License
3.81k stars 1.12k forks source link

Spatial assertions do not quote the name of the spatial involved #1992

Closed richardTingle closed 1 year ago

richardTingle commented 1 year ago

Assertions within the spatial class have no message associated with them. This makes them much less useful for debugging problems (they effectively just say; there is a problem, somewhere in your entire scene graph), By including the spatial name in the assertion message they will help track down the problem much quicker.

(I intend to submit a PR for this)

pspeed42 commented 1 year ago

Note that if you are actually seeing assertions then there are bugs in the engine. Assertions should not be used for user-level errors but double-checking engine state... the cases where if something is wrong then it is really wrong.

Adding messages is good... I've always been a little frustrated that they were never added. Just wanted to point out that if there are cases where we expect the user to care about these then maybe they need a different kind of check.

...we will certainly care, though, and that's where the messages are super-handy.

richardTingle commented 1 year ago

Interesting. I am definitely seeing them. The refreshFlags ones. I'll try and see if I can figure out a pattern (and these messages may help), they are very rare though, maybe only twice in months of development

pspeed42 commented 1 year ago

They are either a sign of engine bugs or engine abuse. Either way, yeah, it would be good to track down why it's happening... and if you could be more specific then I might even speculate.

stephengold commented 1 year ago

Looking at the changes in #1993, my best guess is that the assertion failures are caused by an application updating the scene graph asynchronously. Since there's little JME can do to prevent such abuse, perhaps the assertions should be replaced by IllegalStateException throws like this one:

https://github.com/jMonkeyEngine/jmonkeyengine/blob/76ebd1443ccc18249e9f082d528cd39baf59da84/jme3-core/src/main/java/com/jme3/scene/Spatial.java#L364-L367

richardTingle commented 1 year ago

Yeah, that was what my money was on as well. I use a lot of threading and while I obviously know I'm not supposed to modify the scenegraph on anything but the main thread it's easy to do it accidentally (it's why I religiously give my spatials names so that that IllegalStateException you quote gives helpful answers).

On IllegalStateException vs asserts. Is there an issue with performance, in that the asserts are free in deployed applications but test in development. Whereas an IllegalStateException's checks have to run every time

pspeed42 commented 1 year ago

To me, if the cause turns out to be thread-abuse then I saw leave the assertions. IllegalStateExceptions are a lot more expensive than compiled-out assertions. Even the act of adding a throw new Exception to your code wires in a bunch of infrastructure during compilation that wouldn't normally be there.

Edit: moreover, you might consider posting the forum about your practices that lead this to be "really easy" to do... because we might be able to offer advice. Some more disciplined idioms might help in the future.