Fix skinning initialization in MeshInstance when loaded from thread #48217
Conversation
Fix for a regression from software skinning support: instance_attach_skeleton wasn't called in set_mesh before, and it's causing issues when the mesh instance is loaded from a thread. 1. Call from a thread queues instance_attach_skeleton with RID() in the visual server. 2. Call from the main thread when entering tree calls instance_attach_skeleton immediately with a valid skeleton 3. Queued instance_attach_skeleton resets the attached skeleton This change prevents that to happen by making sure instance_attach_skeleton is not called on set_mesh as it was doing before, but there might be a more general problem to solve in how visual server commands are executed when resources are loaded from a different thread.
|
Agree, that sounds a good explanation - that the underlying cause is different call order due to resources loading from a thread. It is possible it was working before 'by accident' rather than by design, and by making these changes we are re-establishing the accident that allows them to be called in the wrong order. I can't independently review as these are basically the changes I came up with (and it was more applying a band aid than knowing the cause). So maybe @clayjohn can take a look. It might be we would have to ask reduz to confirm / deny the out of order calls idea, but whether he originally wrote this I don't know, and it may not be fresh in his memory. |
There was a problem hiding this comment.
Looks good to me.
@pouleyKetchoupp I agree with your concerns, we may need to discuss the server API more generally with reduz to ensure these sorts of bugs don't crop up.
|
Thanks! |
|
Cherry-picked for 3.3.1. |
Fixes #48152 based on suggestion by @lawnjelly in #48152 (comment)
This is a fix for a regression from software skinning support (PR #40313):
instance_attach_skeletonwasn't called inset_meshbefore, and it's causing issues when the mesh instance is loaded from a thread.instance_attach_skeletonwithRID()in the visual server.instance_attach_skeletonimmediately with a valid skeletoninstance_attach_skeletonresets the attached skeletonThis change prevents that to happen by making sure
instance_attach_skeletonis not called onset_mesh, the same way it was done before #40313.This fix seems to be fine for the specific case in #48152. I've tested with different projects from #40313 and things seem to still work correctly.
But:
There might be a more general problem to solve in how server commands are executed when resources are loaded from a different thread.
At the moment, they can be executed in the wrong order if the first ones are queued and the next ones are executed immediately. This could have other unpredictable side effects.
Maybe queued commands should be flushed when an immediate command is executed? I'm not sure if that would be ok for performance but it would make things much safer.