Fix Scene hot reloading.#18358
Conversation
|
Your PR caused a change in the graphical output of an example or rendering test. This might be intentional, but it could also mean that something broke! If it's expected, please add the M-Deliberate-Rendering-Change label. If this change seems unrelated to your PR, you can consider updating your PR to target the latest main branch, either by rebasing or merging main into it. |
3c2f024 to
8ef8178
Compare
|
Looks like this PR isn't quite ready. I've got two meshes. Adding an empty node and saving after that causes a panic: This also causes the entire app to freeze and blast my CPU usage to 50% (IIRC half the threads are for systems and half for assets, so I guess all the system threads are blasting). Initial investigation shows the hierarchy is not set up correctly, but I'm not sure why yet. |
|
Hi, uh, are you planning on continuing to work on this? |
|
Hi, so sorry I never actually went and updated the status here. This is somewhat blocked on #18418. I say somewhat because it's not clear to me that entities in a scene should be reused at all (which is what that issue is about). Entities in a Scene do not have any semantic information. That means when you change the scene, the entities before and after have no correspondence. Imagine you have the following Scene (which comes from loading a GLTF) If you go and delete the cube, you get the Scene: Note that the sphere now uses the cube's Entity. That means hot reloading would try to turn the cube entity into the sphere. Any components the cube had that the sphere doesn't will not be replaced (for example, if the cube had an IAmAnItemPickup component, now the sphere does. What if the sphere was actually the floor of your world!). This whole problem is exacerbated by relations. I think one approach is to just delete all the entities and spawn new ones. It makes hot reloading less powerful, but makes it behave more reasonably IMO. I can try drafting that up soon. The alternative is this being blocked on #18418 haha. |
|
Actually, maybe even that alternative is a non-starter since if you had something parented to your scene, it would be despawned when you despawned all your entities... So this PR might just be blocked... |
|
Your PR caused a change in the graphical output of an example or rendering test. This might be intentional, but it could also mean that something broke! If it's expected, please add the M-Deliberate-Rendering-Change label. If this change seems unrelated to your PR, you can consider updating your PR to target the latest main branch, either by rebasing or merging main into it. |
|
Alright, after some discussion in Discord, I've been convinced that deleting the scene and respawning it is better than doing nothing. I also added a test for scenes and dynamic scenes to make sure they continue to hot-reload. Note: I've fixed the rendering change, so we don't need the label! |
| for id in scene_ids { | ||
| if let Some(spawned_instances) = self.spawned_scenes.get(id) { | ||
| for instance_id in spawned_instances { | ||
| if let Some(instance_info) = self.spawned_instances.get_mut(instance_id) { | ||
| Self::spawn_sync_internal(world, *id, &mut instance_info.entity_map)?; | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Nothing blocking, but this could be simplified a lot by using .filter_map for the spawned scenes, followed by .flat_map for the spawned instances, followed again by filter_map for the instance info ;)
There was a problem hiding this comment.
I would prefer not to do this to keep it consistent with the dynamic one, and I'd prefer not to change that one to keep the diff small. bevy_scene could use a style pass IMO
Noxmore
left a comment
There was a problem hiding this comment.
Code looks good, and works with my test case (bevy_trenchbroom)!
|
seems to work for blender/skein. speedblender.mp4One thing I noticed is that the entity list seems to grow with every reload. The code here loops over all 
edit: notably the Character Component here is added via basically |
|
@ChristopherBiscardi, could I see your observer code? Is it possible you're spawning an entity instead of adding a component? Hotreloading literally despawns every entity associated with the scene, so I don't know what entities your component could be living off of? bevy_inspector_egui might also shed some light on this. I mean if the entire hierarchy is duplicated after hot reload, then we'll know this PR isn't working. |
The observer code isn't afaik unless something changed significantly between 0.16 and main. The observer code is: https://github.com/rust-adventure/skein/blob/3d5949b7c21fc1b2e54f8414c3fb99e5c4885759/src/lib.rs#L272-L274 which hasn't substantially changed other than Trigger -> On
Is there a version of bevy_inspector_egui that works on main? I think the issue is that I have a scene_spawner in AssetEvent::LoadedWithDependencies, the LoadedWithDependencies is called every time the reload happens, which then creates another instance. So not an issue with the PR afaict. on-load -> spawn is a shortcut I use for some examples that I'll just have to take a different approach for. |
|
yeah that was it, I confirmed with a Local that prevented the spawn if already had been spawned. reloading itself is working fine. |
|
I'm comfortable enough with this fix to merge it now. There's nothing crazy going on here, and the regression test is really helpful. |
# Objective - examples `many_foxes` and `morph_targets` have stopped working since #18358 - any case that load several parts of a complex asset and a scene, and do setup on asset load will fail - Fixes #20383 ## Solution - Debounce scene asset events so that we ignore modified event happening too quickly after each others ## Testing - run examples `many_foxes` or `morph_targets` ## Alternative 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: - Revert #18358. Normal scene loading is more important than hot reloading - Implement partial asset loading in the asset server and the gltf loader so that when we load `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 - Have the asset server load parent assets only once, even when loading subassets. This would be a good change and less complex, but I think worse than the previous idea and kinda not compatible with it --------- Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
Objective
Solution
SceneSpawnerfunctions to include a_dynamicsuffix where appropriate.SceneSpawner.SceneAssetEvents and update the corresponding spawned scenes.InstanceInfo, so that we know where to parent the new entities from a hot-reloaded scene.InstanceInfoprivate (none of our APIs returned it anyway).Testing
hot_asset_reloadingexample and mutated the GLTF to see it be updated. It works! Moving around meshes works in Blender works, deleting meshes also seems to work.Migration Guide
SceneSpawnerhave been renamed:despawn->despawn_dynamicdespawn_sync->despawn_dynamic_syncupdate_spawned_scenes->update_spawned_dynamic_scenesdespawn,despawn_sync, andupdate_spawned_sceneswhich all act onScenes (as opposed toDynamicScenes).