-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Internal Entities #20204
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
Internal Entities #20204
Conversation
|
There are two options for the tests, and I would like some comments as to what would be better:
|
If we can, I think it would be better to rewrite those tests to stop using (Copied from my reply on Discord.) |
|
It looks like your PR is a breaking change, but you didn't provide a migration guide. Please review the instructions for writing migration guides, then expand or revise the content in the migration guides directory to reflect your changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally on board but:
- I have some simple doc suggestions.
- I feel strongly about using queries in our tests rather than counting the total number of entities anywhere. I'll help yell at people too!
|
Apparently this PR does not need a migration guide, but changes to make observers etc use this will. |
|
I've thought about it some more, and I came to agree. would be wonderful to have, but since I'm not going to re-architect queries right now, I'll take the alternative. |
|
Hmm, I think there are some tricks that could make a |
# Objective - `Allows` was added as a handy tool to work with default query filters. - It wasn't in the docs though, and I had to hunt it down while reviewing #20204. ## Solution - Add some breadcrumbs to the docs in the expected places.
# Objective Many tests currently check against `entity_count()` in order to see if the absolute number of entities are where they expect it to be. This is not a very robust of doing it, as we're adding more internal entities (#20204). ## Solution Replace `entity_count()` by a relevant query. We also change `iter_entities` to filter out Internal entities by default. I don't know if this is the right approach, because unlike default query filters, there are very few escape hatches for users who do want to iterate through all entities, including the internal ones. I guess you could just make a query. Actually, why isn't `iter_entities` just a query? ## Testing I added a `world.spawn(Internal)` during world bootstrap and most tests succeeded still.
# Objective Despite initially advocating for its inclusion in #20204, I've been increasingly unconvinced by the edge cases and user-facing complexity and surprise that `Internal` brings. Accidental queries are quite hard to write, and the entitiy-inspector concerns are really a UX problem for each tool to solve that `Internal` doesn't help with. @cart feels similarly: as a result I'm marking this PR as X-Blessed. Closes #21363. ## Solution - Remove `Internal` as a type. - Follow the compiler errors to remove all references. - Write a migration guide.
# Objective Despite initially advocating for its inclusion in bevyengine#20204, I've been increasingly unconvinced by the edge cases and user-facing complexity and surprise that `Internal` brings. Accidental queries are quite hard to write, and the entitiy-inspector concerns are really a UX problem for each tool to solve that `Internal` doesn't help with. @cart feels similarly: as a result I'm marking this PR as X-Blessed. Closes bevyengine#21363. ## Solution - Remove `Internal` as a type. - Follow the compiler errors to remove all references. - Write a migration guide.
Objective
As we move more stuff to entities, it's a good idea to keep these entities quasi-private. We do not want to confuse users by having to explain everything as being an entity.
This came out of #19711.
Solution
This PR introduces the concept of internal entities, entities marked by the
Internalcomponent, that are filtered out by queries throughDefaultQureyFiltersand also don't show up forWorld::entity_count().Testing
Added a test.