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

Observer entities should have names #16384

Open
msvbg opened this issue Nov 14, 2024 · 13 comments
Open

Observer entities should have names #16384

msvbg opened this issue Nov 14, 2024 · 13 comments
Labels
A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use D-Trivial Nice and easy! A great choice to get started with Bevy S-Needs-Design This issue requires design work to think about how it would best be accomplished X-Contentious There are nontrivial implications that should be thought through

Comments

@msvbg
Copy link
Contributor

msvbg commented Nov 14, 2024

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

Observers are spawned as entities. When using a tool like the egui inspector, observer entities are listed under the generic "Entity" name which makes it difficult to understand what they are at a glance

What solution would you like?

Observers should spawn with a Name component which indicates that they are observers.

@msvbg msvbg added A-ECS Entities, components, systems, and events C-Feature A new feature, making something new possible C-Usability A targeted quality-of-life change that makes Bevy easier to use D-Trivial Nice and easy! A great choice to get started with Bevy and removed C-Feature A new feature, making something new possible labels Nov 14, 2024
@alice-i-cecile
Copy link
Member

This is partly a bug with bevy-inspector-egui, in that it should special-case Observer entities like it does for other classes of objects. But yeah, we should use the system name ideally.

@mgi388
Copy link
Contributor

mgi388 commented Nov 14, 2024

This should be easy to fix in inspector egui. See my recent commit fixing lights associations. jakobhellermann/bevy-inspector-egui@8b684b3

If someone doesn’t get to it first I’ll try and do a PR over there, separately to this ticket.

@alice-i-cecile alice-i-cecile added the S-Ready-For-Implementation This issue is ready for an implementation PR. Go for it! label Nov 14, 2024
@BenjaminBrienen
Copy link
Contributor

Does adding a Name component to every Observer create significant overhead for the sake of a a feature which could be implemented instead by the editor GUI? In other words, can the editor just see what the system name is and use that for Observers? (Special-cased)

@msvbg
Copy link
Contributor Author

msvbg commented Nov 14, 2024

Does adding a Name component to every Observer create significant overhead for the sake of a a feature which could be implemented instead by the editor GUI? In other words, can the editor just see what the system name is and use that for Observers? (Special-cased)

I sincerely doubt it! Observers are not spawned often and there should not be that many of them in a game. Personally I think it's good practice for every entity to have a name, and strive to enforce this in my projects, but this may not be standard in the community. Though I'm fine with any solution.

@alice-i-cecile
Copy link
Member

I think we should aim to use Name more aggressively. Long-term, I'd like to move more of our name-like identifiers (like system names) out into the proper Name component.

@BenjaminBrienen
Copy link
Contributor

In that case, maybe bevy_cli could recommend that with a lint?

@alice-i-cecile
Copy link
Member

Yeah, I would say that's a good style level lint.

@mockersf
Copy link
Member

Putting names on everything means allocating strings for everything. They are nice to debug, but will have a cost

@alice-i-cecile alice-i-cecile added the X-Contentious There are nontrivial implications that should be thought through label Nov 15, 2024
@benfrankel
Copy link
Contributor

benfrankel commented Nov 15, 2024

FWIW Rust makes it easy to put Name components behind a feature flag:

commands.spawn((
    #[cfg(feature = "entity_names")]
    Name::new("Foo"),
    Foo,
));

@msvbg
Copy link
Contributor Author

msvbg commented Nov 16, 2024

Putting names on everything means allocating strings for everything. They are nice to debug, but will have a cost

This cost should be very minimal in the grand scheme of things. In my project I have a few dozen observers, and the total memory allocation for names would probably be somewhere south of 1kb. I struggle to see how this would be significant even for a much larger project.

FWIW Rust makes it easy to put Name components behind a feature flag:

It could be feature gated but I don't think this should be this big of a big deal, and Bevy should not offer more obscure flags than it has to. I could see it being gated by #[cfg(debug_assertions)].

@alice-i-cecile
Copy link
Member

Yeah, I'd love to see this unified under a dev_mode feature or something.

@mockersf
Copy link
Member

Should this use the same Name component we already have? This component is used for animations, and fast lookup (faster than string comparison). The Name component we already have could actually not hold a name and still work the same way.

Maybe a QueryableName and a DisplayName components?

@alice-i-cecile
Copy link
Member

Yeah, I would prefer to avoid using the component named Name for animations if the performance/UX requirements are distinct.

@alice-i-cecile alice-i-cecile added S-Needs-Design This issue requires design work to think about how it would best be accomplished and removed S-Ready-For-Implementation This issue is ready for an implementation PR. Go for it! labels Nov 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use D-Trivial Nice and easy! A great choice to get started with Bevy S-Needs-Design This issue requires design work to think about how it would best be accomplished X-Contentious There are nontrivial implications that should be thought through
Projects
None yet
Development

No branches or pull requests

6 participants