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

Bevy internal entities need public visible marker. #12852

Closed
brandon-reinhart opened this issue Apr 2, 2024 · 11 comments
Closed

Bevy internal entities need public visible marker. #12852

brandon-reinhart opened this issue Apr 2, 2024 · 11 comments
Labels
A-Cross-Cutting Impacts the entire engine A-ECS Entities, components, systems, and events C-Feature A new feature, making something new possible

Comments

@brandon-reinhart
Copy link
Contributor

What problem does this solve or what need does it fill?

As far as I know, RegisteredSystem entities are the first internal entity in bevy. Unfortunately, these have no property that makes them distinguishable as "owned by an engine service." Systems that clear large numbers of entities on state change (for example) will sweep up and nuke RegisteredSystems.

(In my case, this means by RegisteredSystems are blown away as soon as the game is loaded.)

What solution would you like?

A simple solution that also keeps internals private is to mark all Bevy owned entities with a common marker.

#[derive(Component)]
pub struct SystemEntity;

This way these entities can be selectively excluded by end users.

What alternative(s) have you considered?

Another possibility is to not keep internal components secret, but the above solution is meant to maximize keeping internal details hidden while also giving the user a hook to interact with system owned entities.

@brandon-reinhart brandon-reinhart added C-Feature A new feature, making something new possible S-Needs-Triage This issue needs to be labelled labels Apr 2, 2024
@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events A-Cross-Cutting Impacts the entire engine and removed S-Needs-Triage This issue needs to be labelled labels Apr 2, 2024
@alice-i-cecile
Copy link
Member

alice-i-cecile commented Apr 2, 2024

Window entities are another place where I would use this marker. #12851 / #12770 is another example, and assets-as-entities and systems-as-entities would both benefit from this.

@SkiFire13
Copy link
Contributor

With more x-as-entities I feel like the converse solution would work better: mark all the entities you are interested in and despawn only them.

@brandon-reinhart
Copy link
Contributor Author

With more x-as-entities I feel like the converse solution would work better: mark all the entities you are interested in and despawn only them.

Either way, I need a way to tell what is system owned. Whether Persistent or NotPersistent. I could not add these by hand to every entity I spawn. If I missed even one, I'd get hard to diagnose bugs.

@rlidwka
Copy link
Contributor

rlidwka commented Apr 2, 2024

Systems that clear large numbers of entities on state change (for example) will sweep up and nuke RegisteredSystems.

Please no.

Systems that clear large number of entities should work on a whitelist, not on a blacklist. You mark your own entities when you create them, then only remove those entities.

Otherwise your code is gonna break every 3rd party plugin that decides to add an entity for internal use.

@brandon-reinhart
Copy link
Contributor Author

brandon-reinhart commented Apr 2, 2024

Systems that clear large numbers of entities on state change (for example) will sweep up and nuke RegisteredSystems.

Please no.

Systems that clear large number of entities should work on a whitelist, not on a blacklist. You mark your own entities when you create them, then only remove those entities.

Otherwise your code is gonna break every 3rd party plugin that decides to add an entity for internal use.

I tend not to use third party plugins these days (too many are abandoned) so I didn't think about that. That example aside, don't I still need to know what bevy owns?

Also... I think I disagree with your claim. Usually there are a handful of things to persist, whereas 99% will be expunged when the game state is exited back to the main menu. Marking everything as Transient is pretty junky.

@rlidwka
Copy link
Contributor

rlidwka commented Apr 2, 2024

I could not add these by hand to every entity I spawn. If I missed even one, I'd get hard to diagnose bugs.

No, you can add those automatically with your own custom 5-line EntityCommands function (or whatever you're using to add them). Avoiding code duplication is a basic programming skill.

That example aside, don't I still need to know what bevy owns?

I'd guess that you don't. It might be used by folks in bevy_inspector_egui to highlight bevy entities in a pretty pink color, but whether that's worth maintenance burden is an open question.

Marking everything as Transient is pretty junky.

You don't mark everything as Transient, you mark 99% of entities that you know for sure are Transient. Why's that junky?

@SkiFire13
Copy link
Contributor

I could not add these by hand to every entity I spawn. If I missed even one, I'd get hard to diagnose bugs.

In the same way if Bevy or some plugin missed even one you would still get hard to diagnose bugs, with the increased downside that you now have to debug code that's not yours.

@james7132
Copy link
Member

I'd be in favor of marking components (in ComponentInfo) with additional metadata, like "managed by bevy_ecs" or "managed by bevy_window". That way systems that operate generically over all entities can be a bit more intelligent about how they work with those entities and resources.

We'll likely need something like this for the editor, where we probably don't want to be surfacing the Window entities in an scene view.

@alice-i-cecile alice-i-cecile reopened this Apr 2, 2024
@alice-i-cecile
Copy link
Member

I do think we want easier to use abstractions for entity cleanup too. I have a design cooking that adds and removes observers in concert with states to correctly mark and clean up entities that I'm happy with. Needs more ECS features first though.

@UkoeHB
Copy link
Contributor

UkoeHB commented Apr 3, 2024

Also... I think I disagree with your claim. Usually there are a handful of things to persist, whereas 99% will be expunged when the game state is exited back to the main menu. Marking everything as Transient is pretty junky.

This issue is a great example of why I want world swapping.

@UkoeHB UkoeHB mentioned this issue Apr 3, 2024
6 tasks
github-merge-queue bot pushed a commit that referenced this issue Jun 4, 2024
…-scoped entities (#13649)

# Objective

Move `StateScoped` and `log_transitions` to `bevy_state`, since they're
useful for end users.

Addresses #12852, although not in the way the issue had in mind.

## Solution

- Added `bevy_hierarchy` to default features of `bevy_state`.
- Move `log_transitions` to `transitions` module.
- Move `StateScoped` to `state_scoped` module, gated behind
`bevy_hierarchy` feature.
- Refreshed implementation.
- Added `enable_state_coped_entities<S: States>()` to add required
machinery to `App` for clearing state-scoped entities.


## Changelog

- Added `log_transitions` for displaying state transitions.
- Added `StateScoped` for binding entity lifetime to state and app
`enable_state_coped_entities` to register cleaning behavior.

---------

Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
Co-authored-by: François Mockers <francois.mockers@vleue.com>
@alice-i-cecile
Copy link
Member

I'm going to close this as not-planned for now and we'll see how the state-scoped entities work to tackle this problem. If it's not sufficient, we can reopen this or make a new issue.

@alice-i-cecile alice-i-cecile closed this as not planned Won't fix, can't repro, duplicate, stale Jun 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Cross-Cutting Impacts the entire engine A-ECS Entities, components, systems, and events C-Feature A new feature, making something new possible
Projects
None yet
Development

No branches or pull requests

6 participants