debounce asset events for scene reloading#20430
Conversation
I agree with this: can you make an issue please? |
|
I would prefer not to rebreak scene hot reloading, but we definitely need a fix. I have a weak preference to this over reverting, and I think it's better than trying to do a more serious fix now. |
|
There is already an issue for this partial asset loading problem #12756. (I will review this when I have time) |
Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
andriyDev
left a comment
There was a problem hiding this comment.
Should we even "fix" this? The problem in the examples is just that they have a system that only ever runs once. Instead, we could change those examples to use the instance ready observer. Practically I would expect most users to be using the observer pattern, and encouraging them to use it would also be good I think.
I'll not block on this, but I think that's my preferred solution.
alice-i-cecile
left a comment
There was a problem hiding this comment.
I agree with @andriyDev's comment about TODOs in the tests and am elevating it to blocking, but otherwise I agree with this strategy.
I can update the examples - I also think they should be using observers as good practice, separately to this issue. I'll make a PR tomorrow unless anyone objects. On whether this should even be fixed, as a user I think it should be. The examples are simple - more complex cases might have chains of events running off scene instancing. Having to make that handle "your scenes may spawn twice... sometimes" would be painful. |
…0531) ## Objective Change some examples to follow best practices for playing animations, and as a bonus work around an issue with scenes spawning multiple times. ## Background Examples that play skeletal animations usually have two steps: 1) Start scene spawning. 2) Play animations after the scene has spawned. Different examples use different approaches for triggering part 2, including a scene spawning observer and `Added<AnimationPlayer>` queries. The observer approach is arguably best as it's more tightly scoped and easier for users to extend. The other approaches work in simple examples but fall down when users want multiple scenes or animations. See #17421 for more detail. As a bonus, the scene spawning observer works around a current issue with scenes spawning multiple times - see #20393, #20430. Although there's an argument that this PR shouldn't land until the issue is properly fixed, as these examples are a useful test case. ## Solution Update the `morph_targets` and `many_foxes` examples to use observers. I also made a few tweaks and fixes to `morph_targets`. - Fix documentation referring to an `update_weights` feature that isn't in the example. - Use the same `AnimationToPlay` component as the `animated_mesh` example. - Change the `name_morphs` system to be event driven and print the asset name. - This is maybe too complex, but could also be nice for users to c&p into their app for debugging. I haven't updated the `animated_mesh_control`, `animated_mesh_events`, and `animation_masks` examples, which still use `Added<AnimationPlayer>`. ## Testing ```sh cargo run --example morph_targets cargo run --example many_foxes ```
|
#18358 also introduced a 15% FPS reduction in |
|
@mockersf That's very surprising, since hot reloading literally deletes all the entities when hot reloading? I don't see how that would really in such a drastic performance degradation... If there is such a degradation, that worries me if there are multiple of the scenes spawned... |
|
I haven't investigated why, but it's possible something is not cleaned correctly. At a first glance everything seems OK on entities, so my bet would be on assets. It's possible some assets are not correctly cleaned with rapid spawn/despawn, and may still be present GPU side in multiple copies |
Objective
many_foxesandmorph_targetshave stopped working since Fix Scene hot reloading. #18358many_foxesand other animation examples have stopped working #20383Solution
Testing
many_foxesormorph_targetsAlternative
I don't think this is a good long term fix, but this should be fixed in the 0.17 and I think alternatives are worse / too complex.
Alternatives are:
file.gltf#Animation0, only the animation data is loaded. This would be a very good change, but too big to do for the 0.17