Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AsyncSceneInheritOutline doesn't propagate outline component removal. #34

Open
rparrett opened this issue Mar 12, 2024 · 5 comments
Open

Comments

@rparrett
Copy link

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
Copy link
Owner

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
Copy link
Contributor

Shatur commented Mar 14, 2024

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

@rparrett
Copy link
Author

rparrett commented Mar 14, 2024

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
Copy link
Contributor

Shatur commented Mar 14, 2024

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.

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
Copy link
Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants