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.

Sign up for GitHub

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

Jump to bottom

Entity{Ref,Mut}Except is duplicate functionality to FilteredEntity{Ref,Mut} #17784

Open
anlumo opened this issue Feb 10, 2025 · 6 comments
Open

Entity{Ref,Mut}Except is duplicate functionality to FilteredEntity{Ref,Mut} #17784

anlumo opened this issue Feb 10, 2025 · 6 comments
Labels
A-ECS Entities, components, systems, and events A-Reflection Runtime information about types C-Bug An unexpected or incorrect behavior D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes S-Needs-Design This issue requires design work to think about how it would best be accomplished

Comments

@anlumo
Copy link
Contributor

anlumo commented Feb 10, 2025

Bevy version

0.15.2

What you did

Working on animation stuff.

What went wrong

Entity{Ref,Mut}Except are missing some functionality I need, like get_by_id. However, it appears that these types aren't used anywhere except the animation system, and it should be possible to replace them with the existing FilteredEntity{Ref,Mut}, which has all the functions I need.
@anlumo anlumo added C-Bug An unexpected or incorrect behavior S-Needs-Triage This issue needs to be labelled labels Feb 10, 2025
@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events A-Reflection Runtime information about types S-Needs-Design This issue requires design work to think about how it would best be accomplished D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes and removed S-Needs-Triage This issue needs to be labelled labels Feb 10, 2025
@alice-i-cecile
Copy link
Member

I'd like to merge these back together, but we have to be careful to ensure that the existing use cases are still supported.

@pcwalton
Copy link
Contributor

I added EntityMutExcept because I first tried to use FilteredEntityMut and it didn't work because you couldn't denylist components, only allowlist them. Animations can animate practically anything, so we need a denylist.

@anlumo
Copy link
Contributor Author

anlumo commented Feb 10, 2025 •
edited
Loading

Calling Access::read_all_components first inverts the behavior to a blocklist instead of an allowlist.

@chescock
Copy link
Contributor

The "Pointing Fingers: Deduplicating Entity APIs with EntityPtr" working group is planning to merge a lot of the code between these types, so they'll eventually be able to share implementations of methods like get_by_id. I'd vote for just adding the methods you need to EntityMutExcept, since we already have a plan to reduce the maintenance burden in the future.

Note that FilteredEntityMut currently has some performance issues because it has to clone() an Access for each instance. EntityMutExcept embeds that all in the type, so it can be created cheaply. (We may be able to fix that with #15396, which I'm planning to get back in shape now that some of the blockers have been cleared out.)

@anlumo
Copy link
Contributor Author

anlumo commented Feb 11, 2025

Note that FilteredEntityMut currently has some performance issues because it has to clone() an Access for each instance.

I see nothing in the type that would stop a switch to Arc<Access>, but I get that this would still be slower than storing this in the type system.

@anlumo anlumo mentioned this issue Feb 11, 2025
Merged
@anlumo
Copy link
Contributor Author

anlumo commented Feb 11, 2025

I added EntityMutExcept because I first tried to use FilteredEntityMut and it didn't work because you couldn't denylist components, only allowlist them. Animations can animate practically anything, so we need a denylist.

My PR has an implementation for converting from Entity{Ref,Mut}Except to FilteredEntity{Ref,Mut} by constructing that denylist.

github-merge-queue bot pushed a commit that referenced this issue Feb 12, 2025
@anlumo
Add ComponentId-taking functions to Entity{Ref,Mut}Except to mirror F…
267a0d0 
…ilteredEntity{Ref,Mut} (#17800)

# Objective

Related to #17784. The ticket is actually about just getting rid of
`Entity{Ref,Mut}Except` in favor of `FilteredEntity{Ref,Mut}`, but I got
told the unification of Entity types is a bigger endeavor that has been
going on for a while now (as the "Pointing Fingers" working group) and I
should just add the functions I actually need in the meantime.

## Solution

This PR adds all of the functions necessary to access components by
TypeId or ComponentId instead of static types.

## Testing

> Did you test these changes? If so, how?

Haven't tested it yet, but the changes are mostly copy/paste from other
implementations in the same file, since there is a lot of duplicated
functionality there.

## Not a Migration Guide

There shouldn't be any breaking changes, it's just a few new functions
on existing types.

I had to shuffle around the lifetimes in `From<&EntityMutExcept<'a, B>>
for EntityRefExcept<'a, B>` (originally it was `From<&'a
EntityMutExcept<'_, B>> for EntityRefExcept<'_, B>`) to make the borrow
checker happy, but I don't think that this should have an impact on user
code (correct me if I'm wrong).
alice-i-cecile pushed a commit to alice-i-cecile/bevy that referenced this issue Feb 12, 2025
@anlumo @alice-i-cecile
Add ComponentId-taking functions to Entity{Ref,Mut}Except to mirror F…
bd200de 
…ilteredEntity{Ref,Mut} (bevyengine#17800)

# Objective

Related to bevyengine#17784. The ticket is actually about just getting rid of
`Entity{Ref,Mut}Except` in favor of `FilteredEntity{Ref,Mut}`, but I got
told the unification of Entity types is a bigger endeavor that has been
going on for a while now (as the "Pointing Fingers" working group) and I
should just add the functions I actually need in the meantime.

## Solution

This PR adds all of the functions necessary to access components by
TypeId or ComponentId instead of static types.

## Testing

> Did you test these changes? If so, how?

Haven't tested it yet, but the changes are mostly copy/paste from other
implementations in the same file, since there is a lot of duplicated
functionality there.

## Not a Migration Guide

There shouldn't be any breaking changes, it's just a few new functions
on existing types.

I had to shuffle around the lifetimes in `From<&EntityMutExcept<'a, B>>
for EntityRefExcept<'a, B>` (originally it was `From<&'a
EntityMutExcept<'_, B>> for EntityRefExcept<'_, B>`) to make the borrow
checker happy, but I don't think that this should have an impact on user
code (correct me if I'm wrong).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-ECS Entities, components, systems, and events A-Reflection Runtime information about types C-Bug An unexpected or incorrect behavior D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes S-Needs-Design This issue requires design work to think about how it would best be accomplished
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@pcwalton @anlumo @alice-i-cecile @chescock