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

Migrate component_reads_and_writes #16339

Open
BenjaminBrienen opened this issue Nov 10, 2024 · 9 comments · May be fixed by #16348
Open

Migrate component_reads_and_writes #16339

BenjaminBrienen opened this issue Nov 10, 2024 · 9 comments · May be fixed by #16348
Labels
A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use 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
Milestone

Comments

@BenjaminBrienen
Copy link
Contributor

BenjaminBrienen commented Nov 10, 2024

Migrate component_reads_and_writes for the 0.16 release. bevy uses it internally and it is deprecated.

It is being made pub(crate) in #16338. didn't work because it is used in an example. That fact makes this a 0.15 issue :)

@BenjaminBrienen BenjaminBrienen added A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Needs-Design This issue requires design work to think about how it would best be accomplished labels Nov 10, 2024
@BenjaminBrienen BenjaminBrienen changed the title Migrate component_reads_and_writes Migrate component_reads_and_writes Nov 10, 2024
@alice-i-cecile alice-i-cecile added D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes and removed D-Straightforward Simple bug fixes and API improvements, docs, test and examples labels Nov 10, 2024
@alice-i-cecile alice-i-cecile added this to the 0.15 milestone Nov 10, 2024
@alice-i-cecile
Copy link
Member

alice-i-cecile commented Nov 10, 2024

This was deprecated in #15207.

@chescock @pcwalton @SkiFire13, I'm not immediately sure on the migration strategy here. Bevy uses this internally in a few places, and the dynamic example uses it at

.component_reads_and_writes()

I like the enum approach suggested here by @chescock personally.

@bushrat011899 bushrat011899 linked a pull request Nov 11, 2024 that will close this issue
@SkiFire13
Copy link
Contributor

Note that the dynamic example was kinda broken before as well, as it didn't support the case where entity can access all components.

Short term I think the enum approach is the best one, but long term I agree with @pcwalton that this is an abuse of Access and shouldn't be exposed like this. I would instead prefer to see an API that allows you to get the list of components on an entity, and the list of components that are accessible on a FilteredEntity{Ref,Mut}.

I would also consider reworking the dynamic example, since it makes a lot of unsafe assumptions that in practice will not be true (e.g. that all the components that the FilteredEntityRef has access to will be of the custom kind the example creates)

@chescock
Copy link
Contributor

Short term I think the enum approach is the best one, but long term I agree with @pcwalton that this is an abuse of Access and shouldn't be exposed like this.

How is it an abuse? I see Access as a kind of set, and iterating the elements of a set doesn't seem any worse than asking whether it contains a given element.

I do agree that fn component_reads_and_writes(&self) -> (impl Iterator<Item = T> + '_, bool) is bad! But I think it's bad because it leaks the implementation details of how unbounded sets are represented, and returning an enum would clean that up.

@SkiFire13
Copy link
Contributor

How is it an abuse? I see Access as a kind of set, and iterating the elements of a set doesn't seem any worse than asking whether it contains a given element.

I'm ok with just exposing iteration over all components included in this set. But exposing how the set is defined (i.e. by inclusion or exclusion of some element) exposes implementation details.

I do agree that fn component_reads_and_writes(&self) -> (impl Iterator<Item = T> + '_, bool) is bad! But I think it's bad because it leaks the implementation details of how unbounded sets are represented, and returning an enum would clean that up.

I don't see the difference between that function and an enum, both leak the implementation details in the same way. An enum would just be a bit more ergonomic.

@bushrat011899
Copy link
Contributor

#16348 provides one possible way forward. The general idea is to provide a higher level abstraction with fallibility in mind. That extra height allows the implementation details of Access to change while still providing a method that should be flexible enough to adapt.

github-merge-queue bot pushed a commit that referenced this issue Nov 11, 2024
# Objective

- Does not correct #16339 but takes it out of the 0.15 milestone

## Solution

- Make it future us problem instead of solving it now
@mockersf mockersf modified the milestones: 0.15, 0.16 Nov 11, 2024
mockersf added a commit that referenced this issue Nov 12, 2024
# Objective

- Does not correct #16339 but takes it out of the 0.15 milestone

## Solution

- Make it future us problem instead of solving it now
@chescock
Copy link
Contributor

But exposing how the set is defined (i.e. by inclusion or exclusion of some element) exposes implementation details.

Ah, is your worry that we'll introduce new kinds of infinite sets that can't be expressed that way? Like, I can imagine a future "this reads any component that implements a specific trait" access. (I'd consider adding those to be an API change rather than an implementation detail change, but it's a compatibility hazard either way.)

Does @bushrat011899's approach in #16348 alleviate that concern? All infinite sets are collapsed in a single Err(UnboundedAccess) variant, which should allow any possible set to be expressed.

@SkiFire13
Copy link
Contributor

Ah, is your worry that we'll introduce new kinds of infinite sets that can't be expressed that way? Like, I can imagine a future "this reads any component that implements a specific trait" access. (I'd consider adding those to be an API change rather than an implementation detail change, but it's a compatibility hazard either way.)

Yes, I think there are possibilities for changes like that to Access. Backward compatibility is of course always an issue, but there are differences in how to handle a simple breaking change in the API and a fundamental change in what it exposes.

Does @bushrat011899's approach in #16348 alleviate that concern?

Yes, it does solve that concern, however it feels a bit limited as an API. I worry that people will always assume it won't error, or they will replace an error with empty sets etc etc.

@bushrat011899
Copy link
Contributor

Unfortunately the only other way I think we could resolve the unbounded-set issue would be to require the user to provide access to a reference (bounded) set (e.g., Components). Which doesn't work well with the current usage of this method.

@chescock
Copy link
Contributor

I think it might be possible to remove the call to component_reads_and_writes() in QueryBuilder::is_dense(), which might make this easier to resolve. See #16396 for my attempt to do so.

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-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
Development

Successfully merging a pull request may close this issue.

6 participants