komadori / bevy_mod_outline

Apache License 2.0
119 stars 10 forks source link

`AsyncSceneInheritOutline` doesn't propagate outline component removal. #34

Open rparrett opened 6 months ago

rparrett commented 6 months ago

Rather than add OutlineBundle to every type of entity in my app that might at some point be outlined, I was attempting to just add and remove the outline bundle on demand.

commands.entity(item_entity).insert((
    OutlineBundle { /* ... */ },
    AsyncSceneInheritOutline,
));

// And later:
commands
    .entity(item_entity)
    .remove::<(OutlineBundle, AsyncSceneInheritOutline)>();

This works great if I'm manually traversing the hierarchy and operating on the meshe(s) in the scene, but not with AsyncSceneInheritOutline. With AsyncSceneInheritOutline, the outline remains on the entity. The removal isn't propagated to the mesh entities in the scene.

komadori commented 6 months ago

The computed outline is propagated down the hierarchy from entities that match the query (With<ComputedOutline>, Without<InheritOutline>). So, when you remove the root OutlineBundle this propagation no longer happens and the inherited outlines remained in the last propagated state. Cleaning up after missing roots would, in the general case, require an additional pass over the InheritOutline components each frame and I'm not sure about spending the cost of that. My inclination is to consider this inheritance behaviour intentional.

The real issue for you is that adding AsyncSceneInheritOutline is not reversible. It was intended as triggering a one-time setup operation. I'll consider this a feature request that removing an AsyncSceneInheritOutline should iterate through the scene entities and remove all the InheritOutlineBundles.

Shatur commented 6 months ago

Instead of adding and removing the bundle, I would just hide the outline. I assume that's your case, right?

rparrett commented 6 months ago

Instead of adding and removing the bundle, I would just hide the outline.

Discussed in #32, but I don't really want to add outline components to every entity that might ever have an outline when only one entity will ever be outlined at any given time. For my use case, this does not seem ergonomic.

But because that seems to be the intended way to use this library, I don't really consider this a bug. Maybe just a documentation issue.

Feel free to close.

Shatur commented 6 months ago

Discussed in https://github.com/komadori/bevy_mod_outline/issues/32, but I don't really want to add outline components to every entity that might ever have an outline when only one entity will ever be outlined at any given time.

I personally add it to all entities I want to outline. Like I add Transform for all entities that I have a position. Adding and removing is not as convenient and causes archetype moves. But it's up to you and the author, of course :)

rparrett commented 6 months ago

The potential effects of archetype fragmentation are more interesting to me. In my application, removing or adding an outline is something that would happen to at most one entity in a single frame, so tiny perf optimizations for things that happen when enabling or disabling an outline don't seem worth thinking about. Would love to benchmark / stress test some scenarios but I can't be bothered because I don't think it will ever matter on my end.