Skip to content

Conversation

@porkbrain
Copy link
Contributor

Objective

Right now when using egui, systems are inserted without any identifier and to the root. I'd like to name those systems and insert them as children to a root entity. This helps to keep the editor organized.

Solution

  • Although the SystemId is documented as an opaque type, examples depicted above benefit from tear down of the abstraction.

Changelog

Added

  • Implemented From<SystemId> for Entity

…upposed to be an opaque type, it's still useful to label the entity with either custom components or with debug components such as Name.
@github-actions
Copy link
Contributor

github-actions bot commented Feb 7, 2024

Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

@porkbrain porkbrain changed the title Implements conversion from SystemId to Entity. Although SystemId is s… Implements conversion from SystemId to Entity Feb 7, 2024
@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use labels Feb 7, 2024
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a little nervous about doing this, as we may end up breaking this functionality in the future. But I can see the value in your use case.

What if we inserted the Name component to all system entities that matched the system name instead?

@alice-i-cecile alice-i-cecile added the X-Controversial There is active debate or serious implications around merging this PR label Feb 7, 2024
@pablo-lua
Copy link
Contributor

What if we inserted the Name component to all system entities that matched the system name instead

Basically, insert the name of the given system on the entity?
for example: If I register the system "foo", the entity will have name "foo"

@porkbrain
Copy link
Contributor Author

Thanks for getting back to me so quick!

What if we inserted the Name component to all system entities that matched the system name instead?

Adding the Name component automatically would be great. However, I am not sure how to go about it given that bevy_ecs does not depend on bevy_core that exports it. Any pointers to help me with this welcome.

Right now, I am using about a dozen registered systems (to modularize dialog logic), so automatically naming them is sufficient. However, inserting more and more registered systems would eventually warrant a root entity under which I hide all those systems. My plan was to cmd.entity(root).add_child(system_id.into()).
This is purely cosmetic: have egui organized and named instead of a long list of unnamed Entity items.

In any case, if your intuition says it will be refactored in the future, feel free to close the PR, it's a nice-to-have.

@pablo-lua
Copy link
Contributor

IMO, would be good to support some kind of API to help the user, even if this will be refactored in the future
I have an Idea of how to implement something like that, but that would be a little different from the original proposal of inserting the Name

we would first spawn the entity with a given bundle (the user would give the bundle to the function)
something like register_system_with<B: Bundle>(bundle: B, system: /*...*/)
Then, I'm remembering of the register_boxed_system_with_entity (Big name) that was first implemented on a early version of this PR, that would pretty much help with this Bundle insertion (See #11019)
But thats only an Idea that I had

@alice-i-cecile
Copy link
Member

Hmm right, it's stuck over in bevy_core. Even though this is likely to get refactored, I think this is a reasonable direction. @maniwani is hoping to make all systems entities one day, so this would still be effectively possible.

Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should improve these docs, but I'm on board.

@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Feb 8, 2024
Accepted @alice-i-cecile docs suggestion

Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
@alice-i-cecile alice-i-cecile removed the X-Controversial There is active debate or serious implications around merging this PR label Feb 8, 2024
@alice-i-cecile alice-i-cecile removed the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Feb 8, 2024
@porkbrain porkbrain changed the title Implements conversion from SystemId to Entity Implements conversion from SystemId to Entity Feb 9, 2024
Copy link
Contributor

@maniwani maniwani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is OK to allow since the component holding the system trait object is still private.

That said, having visible components will make system entities easier to match in queries, so it'll be easier for someone to delete them by accident, so I guess just watch out for that.

@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Feb 26, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Feb 26, 2024
Merged via the queue into bevyengine:main with commit 43b859d Feb 26, 2024
msvbg pushed a commit to msvbg/bevy that referenced this pull request Feb 26, 2024
# Objective

Right now when using egui, systems are inserted without any identifier
and to the root. I'd like to name those systems and insert them as
children to a root entity. This helps to keep the editor organized.

## Solution

- Although the `SystemId` is documented as an opaque type, examples
depicted above benefit from tear down of the abstraction.

---

## Changelog

### Added
- Implemented `From<SystemId>` for `Entity`

---------

Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
msvbg pushed a commit to msvbg/bevy that referenced this pull request Feb 26, 2024
# Objective

Right now when using egui, systems are inserted without any identifier
and to the root. I'd like to name those systems and insert them as
children to a root entity. This helps to keep the editor organized.

## Solution

- Although the `SystemId` is documented as an opaque type, examples
depicted above benefit from tear down of the abstraction.

---

## Changelog

### Added
- Implemented `From<SystemId>` for `Entity`

---------

Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
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 S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants