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

[Merged by Bors] - implement DetectChanges for NonSendMut #2326

Closed

Conversation

NathanSWard
Copy link
Contributor

Objective

  • The DetectChanges trait is used for types that detect change on mutable access (such as ResMut, Mut, etc...)
  • DetectChanges was not implemented for NonSendMut

Solution

  • implement NonSendMut in terms of DetectChanges

@github-actions github-actions bot added the S-Needs-Triage This issue needs to be labelled label Jun 9, 2021
@NathanSWard NathanSWard added C-Code-Quality A section of code that is hard to understand or change A-ECS Entities, components, systems, and events and removed S-Needs-Triage This issue needs to be labelled labels Jun 9, 2021
MinerSebas added a commit to MinerSebas/bevy that referenced this pull request Jun 9, 2021
@mockersf
Copy link
Member

mockersf commented Jun 9, 2021

could you do the same for NonSend (in the same file)?

@cart
Copy link
Member

cart commented Jun 9, 2021

That can't currently be done because these impl macros assume things like DerefMut and set_changed. There is definitely an argument to be made that we should move DetectChanges::set_changed into its own trait though, to enable implementing DetectChanges for read only pointers.

@cart
Copy link
Member

cart commented Jun 9, 2021

Short term I think we should just merge this as-is

@cart
Copy link
Member

cart commented Jun 9, 2021

bors r+

@NathanSWard
Copy link
Contributor Author

There is definitely an argument to be made that we should move DetectChanges::set_changed into its own trait though, to enable implementing DetectChanges for read only pointers.

Yep! Sure I like this idea.
I'll make a followup PR that tries to abstract these out :)

bors bot pushed a commit that referenced this pull request Jun 9, 2021
# Objective

- The `DetectChanges` trait is used for types that detect change on mutable access (such as `ResMut`, `Mut`, etc...)
- `DetectChanges` was not implemented for `NonSendMut`

## Solution

- implement `NonSendMut` in terms of `DetectChanges`
@bors bors bot changed the title implement DetectChanges for NonSendMut [Merged by Bors] - implement DetectChanges for NonSendMut Jun 9, 2021
@bors bors bot closed this Jun 9, 2021
@NathanSWard
Copy link
Contributor Author

NathanSWard commented Jun 9, 2021

That can't currently be done because these impl macros assume things like DerefMut and set_changed. There is definitely an argument to be made that we should move DetectChanges::set_changed into its own trait though, to enable implementing DetectChanges for read only pointers.

@cart As a quick clarification, only the mutable variants ResMut NonSendMut etc.. support the set_changed functionality?

And lastly, are there any other structs other than NonSend that support change detection in some form?

@cart
Copy link
Member

cart commented Jun 9, 2021

@cart As a quick clarification, only the mutable variants ResMut NonSendMut etc.. support the set_changed functionality?

Correct. set_changed requires unique mutable access to a component, so only mutable pointers can safely do this

And lastly, are there any other structs other than NonSend that support change detection in some form?

Res also supports "read only" change detection.

ostwilkens pushed a commit to ostwilkens/bevy that referenced this pull request Jul 27, 2021
# Objective

- The `DetectChanges` trait is used for types that detect change on mutable access (such as `ResMut`, `Mut`, etc...)
- `DetectChanges` was not implemented for `NonSendMut`

## Solution

- implement `NonSendMut` in terms of `DetectChanges`
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-Code-Quality A section of code that is hard to understand or change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants