How to prevent modifying Spatial out of main thread?

EDIT: 1ST: read best practices that also says “Only the tranformation code that actually modifies the spatial must be called from a control, or must be enqueue()ed.”

EDIT: go straight here, this code seems to provide such detection!

I am trying to find a way to detect if a Spatial was modified outside of the main thread and promptly throw an exception (so we know how/where/when it was modified).

I was going to prevent access to SimpleApplication.getRootNode() if not at main thread, but that seems to much (as I understand, many of its functionalities cause no trouble out of main thread).

I tried to put a breakpoint everywhere that changes Spatial.refreshFlags with this conditional:
!Thread.currentThread().getName().equals("jME3 Main")
but that is too encumbersome to the IDE…

For reference, the exception happens at:

Spatial.checkCulling()
TerrainPatch.collideWith()

Check this topic also, it is about a better error message concerning the exception about spatial modifications out of main thread.

Here I created a patcher that detects refreshFlags modification out of main thread, but I think that code is not good enough, it could be better improved by checking if the thread object was compared against the renderer thread as done here, but I am not still sure if my “spaghetti code patcher” will be really precise/helpful/adequate.

Personnaly I’ve got pools for Spatials, where I store references. These pools have accessors, so you could add assertion for what should be the current thread into it.

Be careful. Bloating your design in order to assure architectural design patterns are adhered to through code always leads to disaster in the long run. Explaining this warning in terms of cause and effect is beyond the scope of this post. Basically your approach to solving your problem should not be code but an adherence to design patterns through your implementation.

1 Like

@methusalah

Sounds interesting. Do you mean like a list of spatials to be checked at other thread loops or before they exit? In this case I could use reflection to check that flag as it is not accessible, could do the trick! like in checkIfModified(spatial1, spatial2, ...)

@zissis

In other words, you mean I must know all the methods that modify that flag and avoid using them outside of the main thread? Or I should more widely avoid coding certain things outside of the main thread, that, of course, also involves that flag, like a coding standard concerning the way JME works?

Anyway, my argument is mainly concerned about knowing the specific location where the problem was originated without spending hours on it. If that could be addressed in some way, that will help a lot!

Initially I thought that all of these points could have a protection throwing exception in case it is modified improperly, but the main problem on this could be the CPU usage?

@teique I understand what you are trying to do. You made a mistake in your code and modified the scene graph outside of the JME thread and you want to find it in an automated fashion. Here are the problems with that approach:

  1. It will take you longer to create the spaghetti code to throw exception that it would to read through your code and find the bug.

  2. Your spaghetti code will either prevent you from using your scene graph effectively in a multi threaded environment and will never take into account all of the use cases.

  3. If you had the knowledge to create the spaghetti code then you would have never created the current bug in the first place and not need the spaghetti code.

My advice, learn about the scene graph and rendering pipeline so that you understand what you CAN modify and/or access out side of the JME thread and what you can’t. Once you obtain this knowledge you will most likely just have to think though your code and know where the bug is … if it even exists. As a side benefit, you will be a much better JME developer doing it this way and will be able to write code faster in the environment going forward.

1 Like

I agree with others… this is a bad path to go down. Never use a low level check to tell you that your design is bad. It’s just wasteful for one… for another, the check has to be way more extensive than what you have.

It is perfectly fine to modify a spatial from any thread your heart desires UNTIL it is attached to a viewport and actually rendered. Checking for root node is not enough. guINode counts too. Then one day you add another viewport and that viewport counts also.

Really the bug is indicative of a design issue in your multithreading code. Fix that and you won’t have a problem and won’t need the check. If your design is such that is it is not crystal clear when/where you are modifying a spatial in which thread then the design is busted.

2 Likes

@pspeed may be, this option below may be useful to slap newbies faces to implement their (my) code properly :smile: , now, instead of checking for root node or gui node, the aim is towards modification after rootNode.updateGeometricState() and BEFORE the rendering ends. But… the randomicity of that access may prove it is actually worthless. Unless I can find a way to create a thread.sleep() for the other threads whenever the rendering is being performed?

@Empire_Phoenix (about this post where your code seems to provide a way to completely block access to Node methods outside of openGLThread / rendering / mainThread, as long assertion is enabled!)

Cant wait to test it!

Btw, Interestingly enough, I had created a patcher to Spatial.java and other files (using linux sed) that focused on placing the thread check everywhere the refreshFlags variable got modified, just before it, but it always had CPU usage, not like yours, and… it could be more precise as I explain below:

# create the function to be placed where it is important
sed -i.bkp -r 's@private static final Logger logger@protected void assertMainThread(){if(!Thread.currentThread().getName().matches("main|jME3 Main"))throw new NullPointerException("FORBIDDEN modification of spatial outside of main thread!!!");}\n\t&@' ./src/com/jme3/scene/Spatial.java
# place the function at several files matching refreshFlags modification
find ./src/ -type f -iname "*.java" -exec sed -i -r 's@[^[:blank:]/]*[.]{,1}refreshFlags[[:blank:]][|&]=@assertMainThread();&@' {} +
# show the modifications (disabled sed backups above as would create even for unmodified files)
grep assertMainThread ./src/* -irnI

So re-reading the exception message at Spatial.checkCulling():

Scene graph is not properly updated for rendering.\n
State was changed after rootNode.updateGeometricState() call. \n
Make sure you do not modify the scene from another thread!\n
Problem spatial name:

Concerning precision: As I understand, in therms of synchronization, even if I shouldnt, I “can” modify the scene from another thread, as long it happens before rootNode.updateGeometricState(), or after the rendering step completes (right?).

So again with your code, with assertion enabled, we can track if we implemented some Node method access outside of main thread right?
But I believe we could be more flexible and restrict only a few methods using a mix of the code you provided and the sed patcher I made.

  • The problem I still didnt manage to work around is how to synchronize that check to throw exception/assertion only if refreshFlags modification happens AFTER rootNode.updateGeometricState() and BEFORE the rendering ends… But, if the modification happens in a random moment (most probably) it will not be a good access at all, therefore your code will be the best option per se, as we cant grant anything under this kind of randomicity. Unless we can find a way to create a thread.sleep() for the other threads whenever the rendering is being performed? I think such sleep, must happen at every attempt to modify refreshFlags, therefore at assertMainThread(), but I feel it must be a kind of atomic/single/synchronized access implementation, what may clog the main thread and be a bad thing after all…

PS.:
Also, I looked for ThreadSaveNode everywhere here: empirephoenix · GitHub
Can I add it to my project under BSD license?
By what you coded, it looks like we have to determine what nodes will have that checker by instancing them with ThreadSaveNode, so basically I understand that suffices (instead of replacing/patching Node.java, or may be using some kind of classloader trick :)).

Ugh… don’t do this. Just don’t use threads at all in that case. If they are too hard to manage then your design is bad and you should probably do something simpler. Threading is hard. Threading will always be hard. Try writing a few games that don’t use threading to get some experience and then circle back.

At least that’s the impression I get from your bending over backwards to deal with a problem that I’ve personally literally only seen ONE TIME in the entire 5 years I’ve been doing hardcore JME development. And in that case it was an immediate, “Oh yeah, I forget to queue this network message on the main thread.”

NO NO NO NO NO. Spatial is not thread safe. If it’s attached to a viewport then DO NOT TOUCH IT.

A lot of people think “It’s ok to read the properties from different threads even if they aren’t protected, right?” No. Absolutely not. You may see consistent or even partial results. Don’t do it.

You should really create a separate post about your threading design because it sounds way broken.

There is the slightest smallest chance we might expand the engine’s message to include modified spatials in a similar way to what you’ve done. The rest is garbage we will not include in the engine.

Edit: 99.99999% of the time I see this error message it’s because I haven’t handled a viewport properly… and then knowing the root node name was all I needed. I do understand that knowing the exact spatials involved could be useful, though.

1 Like

Note: all of this energy might be better spent discussing your threading design, figuring out how it’s broken, and then writing some good tutorials that will save the next set of users from the same pitfalls.

2 Likes