Jolt Physics warnings for PhysicalBone3D fail to report bone names when running physics on separate thread

[arocull]

Tested versions

• Reproducible in 4.4-beta2, 4.4-beta3
• Not reproducible in 4.3 (no Jolt implementation)

System information

Godot v4.4.beta3 - Fedora Linux 41 (KDE Plasma) on Wayland - X11 display driver, Multi-window, 1 monitor - Vulkan (Forward+) - dedicated NVIDIA GeForce RTX 4080 SUPER (nvidia; 565.77) - AMD Ryzen 7 8700F 8-Core Processor (16 threads)

Issue description

I am using the native Jolt Physics engine on a separate thread via project settings (physics/3d/run_on_separate_thread). I have an armature that needs updating, so Jolt is emitting warnings. However, the warning is attempting to fetch the names of the problem bones, which is not thread-safe, so the warning text is malformed and is passed with multiple errors (as seen below).

ERROR: Caller thread can't call this function in this node (/root/@EditorNode@21257/@Panel@14/@VBoxContainer@15/DockHSplitLeftL/DockHSplitLeftR/DockHSplitMain/@VBoxContainer@26/DockVSplitCenter/@VSplitContainer@54/@VBoxContainer@55/@EditorMainScreen@102/MainScreen/@CanvasItemEditor@11479/@VSplitContainer@11131/@HSplitContainer@11133/@HSplitContainer@11135/@Control@11136/@SubViewportContainer@11137/@SubViewport@11138/RDMOutpost/world/renegade/visual/renegade/Skeleton3D/Physical Bone DEF-spine_001). Use call_deferred() or call_thread_group() instead.
   at: to_string (scene/main/node.cpp:2697)
ERROR: Caller thread can't call this function in this node (/root/@EditorNode@21257/@Panel@14/@VBoxContainer@15/DockHSplitLeftL/DockHSplitLeftR/DockHSplitMain/@VBoxContainer@26/DockVSplitCenter/@VSplitContainer@54/@VBoxContainer@55/@EditorMainScreen@102/MainScreen/@CanvasItemEditor@11479/@VSplitContainer@11131/@HSplitContainer@11133/@HSplitContainer@11135/@Control@11136/@SubViewportContainer@11137/@SubViewport@11138/RDMOutpost/world/renegade/visual/renegade/Skeleton3D/Physical Bone DEF-spine). Use call_deferred() or call_thread_group() instead.
   at: to_string (scene/main/node.cpp:2697)
WARNING: Hinge joint bias limit is not supported when using Jolt Physics. Any such value will be ignored. This joint connects '' and ''.
     at: set_param (modules/jolt_physics/joints/jolt_hinge_joint_3d.cpp:221)

When physics is set to run on the main thread, the problem resolves, showing the warnings as normal, including the bone names (expected behavior).

WARNING: Hinge joint bias limit is not supported when using Jolt Physics. Any such value will be ignored. This joint connects 'Physical Bone DEF-spine:<PhysicalBone3D#3027113173784>' and 'Physical Bone DEF-spine_001:<PhysicalBone3D#3027146728218>'.
     at: set_param (modules/jolt_physics/joints/jolt_hinge_joint_3d.cpp:221)
WARNING: Hinge joint bias limit is not supported when using Jolt Physics. Any such value will be ignored. This joint connects 'Physical Bone DEF-spine:<PhysicalBone3D#3102627426828>' and 'Physical Bone DEF-spine_001:<PhysicalBone3D#3102660981262>'.
     at: set_param (modules/jolt_physics/joints/jolt_hinge_joint_3d.cpp:221)

Should the warning itself be deferred to the main thread?

Steps to reproduce

1. Set physics to run on a separate thread physics/3d/run_on_separate_thread
2. Switch Physics Engine to Jolt Physics
3. Set up a Skeleton3D with PhysicalBone3D bones that have joint properties unsupported in Jolt (i.e. a Pinjoint configured with impulse clamping)
4. Restart editor to see errors in console, or launch game to see them in debugger

Minimal reproduction project (MRP)

Warnings should appear as soon as this project is opened and imported.

physicalbone3d_report_2025-02-09_17-35-51.zip

[mihe]

However, the warning is attempting to fetch the names of the problem bones, which is not thread-safe, so the warning text is malformed and is passed with multiple errors (as seen below).

That is indeed problematic, and happens in more places than just this particular warning.

I'll need to see how to best fix this. The fact that I'm even reporting node names from the physics server to begin with is highly questionable, and arguably breaks the "separation of concerns" between the scene side of things and the servers, but the errors/warnings are somewhat useless without any context.

Ideally they would be actual configuration warnings on the nodes themselves I guess. The current way of doing it is mostly a crude carry-over from the extension, where there weren't a lot of better options, but I also didn't want the Jolt module to "spill over" into other parts of the codebase while it's still experimental.

[smix8]

The fact that I'm even reporting node names from the physics server to begin with is highly questionable, and arguably breaks the "separation of concerns" between the scene side of things and the servers.

Having the node names reported by itself to help with debug is not the problem, but having a server rely back on anything from the SceneTree or nodes is.

So those node name strings would need a copy stored inside the server that maps to whatever server internal object. The server can not ask back anything from the SceneTree outside of very specifically designed callbacks, it is in general the wrong direction.

[mihe]

So those node name strings would need a copy stored inside the server that maps to whatever server internal object.

I guess the issue with that is nodes changing names, or just initializing them late, with no ability (with the current interface) for the server to pick up on that change. I suppose that's a minor concession though, since it's likely to be a niche problem.

The bigger loss would be the inability to provide meaningful errors in release exports, since the stored node names would come at a potentially non-trivial memory cost, and thus rightfully shouldn't be stored in such builds. But seeing as how error-checking in Godot is sometimes compiled out entirely in release exports, I guess there's precedent for that already.

It does seem like the least invasive option though. Any sort of deferral of the error to the main thread would at the very least require a set of custom error macros in order to report the correct file and line, and you're still left with the conceptual problem of grabbing scene stuff from the server at times when it might not be appropriate to do so.

[smix8]

The server should not be responsible to pick up such node changes. It is the node duty to make those changes known to a server.

E.g. could add a generic xyz_set_meta() function for all the RID types to the PhysicsServer API that allows to add random stuff with a Dictionary to them for debug and the nodes could call that everytime stuff like their name changes.

[mihe]

While an API like that does seem mildly useful in general, adding to the physics server interface just for this (and at this late stage) seems unfortunate, and also makes the server error reporting more of a commitment than I intended for it to be.

I would probably prefer to skip the metadata part and just have the server pull the names once, and then we could change/improve this for 4.5 instead.

[akien-mga]

I'd suggest we should properly weigh the cost of adding a potentially heavy or hacky debugging infrastructure in servers, versus simply not providing the extra useful information we'd like to provide here.

It's a design constraint of Godot that once things are at the server level, it's not possible to go back to the scene structure. The Jolt integration inherits this design limitation and should maybe not try to work it around.

A proper debugging infrastructure to be able to go back to scene level when needed for descriptive errors might be worth considering, but this is proposal material and not specific to Jolt warnings.

In this specific case, usage of invalid joint types while using the Jolt backend would be better checked at the node level via Node configuration warnings. Or we just live with only the physics server generic error, and users can grep their scenes and source code to find which scenes use unsupported joints.

[smix8]

For 4.4 I would just reduce the error detail and go with a generic error without any node names or specifics. It is imo too late in the 4.4 cycle for any of those other changes that require more work. They can be done for 4.5 and if kept simple enough maybe even backported with a patch later.

I am not sure how feasible it is to add specific physics backend errors to node config warnings. Imo nodes should not really care what backend is available.

[mihe]

Warnings/errors without context might be fine for this particular warning, since you can indeed (in the worst case) grep for the specific property and maybe find the offending node. There are however a few warnings/errors that relate to numerical values, such as the ones for invalid scaling, and which can end up triggering only at runtime in certain cases. Providing no context for those errors is likely to be frustrating to the point of being detrimental, so I'll probably opt to remove those entirely in that case.

[mihe]

I threw together a PR for removing the various scene contexts: #102717

Let me know what you think.

[mihe]

Alternative (much simpler) fix that just falls back on using <unknown> as the object name when we're threaded: #102726

That would mean the error shown in this issue would instead be:

WARNING: Hinge joint bias limit is not supported when using Jolt Physics. Any such value will be ignored. This joint connects '<unknown>' and '<unknown>'.

... but only when physics/3d/run_on_separate_thread is enabled.

[arocull]

Wow, that was fast! Thanks!