search

komadori / bevy_mod_outline

Apache License 2.0
119 stars 10 forks source link

Add `AsyncSceneOutline` #30

Closed Shatur closed 6 months ago

Shatur commented 7 months ago

bevy_xpbd have AsyncSceneCollider which initializes all colliders from meshes after scene loading.

It would be convenient to provide something like AsyncSceneOutline component and a system that listens for SceneInstanceReady event and for all entities with AsyncSceneOutline inserts InheritOutlineBundle to all their children.

komadori commented 6 months ago

Good idea, thanks. Added by 87df3aaf708e6c64b1340d137aeb481506c01bff.

Shatur commented 6 months ago

@komadori Thanks! But I see that you use SceneSpawner. I think it's slover to iterate over all scenes. Instead you can just listen for SceneInstanceReady events.

komadori commented 6 months ago

@Shatur My thinking at the time was what would happen if you added AsyncSceneInheritOutline after the scene had already loaded. I see now that a system that queried Added<AsyncSceneInheritOutline> could handle that in addition to listening for SceneInstanceReady, although I'm not sure if it's actually worth the bother.

More generally, I'm not entirely happy with the abstraction offered Bevy. Reading the discussion on bevyengine/bevy#9115, I'm not convinced that SceneInstanceReady wouldn't have been better as a marker component than as an event. While I'm not a bevy_ecs expert, I would think that the transient cost of an extra archetype while a scene is loading would be small compared to the cost of actually loading a scene and realising it. This would allow you efficiently query (With<AsyncSceneInheritOutline>, With<SceneInstanceReady>), whereas the event is produced for every scene even if it isn't marked.

Anyway, I'll have a see about changing it to use the event.

Shatur commented 6 months ago

I suggested the marker component initially, but they decided on the event, so I edited the issue. After using it for some time, I found it quite convenient.

komadori commented 6 months ago

Yes, I saw. Thanks for working on it. I think it will use some kind of per-entity event/observer pattern in the future once Bevy has support for it, and that will be the best solution.