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

add MutUntyped::map_unchanged #9194

Merged
merged 13 commits into from
Jul 23, 2023
Merged

add MutUntyped::map_unchanged #9194

merged 13 commits into from
Jul 23, 2023

Conversation

Serverator
Copy link
Contributor

Adopted #6430

Objective

MutUntyped is the untyped variant of Mut<T> that stores a PtrMut instead of a &mut T. Working with a MutUntyped is a bit annoying, because as soon you want to use the ptr e.g. as a &mut dyn Reflect you cannot use a type like Mut<dyn Reflect> but instead need to carry around a &mut dyn Reflect and a impl FnMut() to mark the value as changed.

Solution

* Provide a method `map_unchanged` to turn a `MutUntyped` into a `Mut<T>` by mapping the `PtrMut<'a>` to a `&'a mut T`
  This can be used like this:
// SAFETY: ptr is of type `u8`
let val: Mut<u8> = mut_untyped.map_unchanged(|ptr| unsafe { ptr.deref_mut::<u8>() });

// SAFETY: from the context it is known that `ReflectFromPtr` was made for the type of the `MutUntyped`
let val: Mut<dyn Reflect> = mut_untyped.map_unchanged(|ptr| unsafe { reflect_from_ptr.as_reflect_ptr_mut(ptr) });

Note that nothing prevents you from doing

mut_untyped.map_unchanged(|ptr| &mut ());

or using any other mutable reference you can get, but IMO that is fine since that will only result in a Mut that will dereference to that value and mark the original value as changed. The lifetimes here prevent anything bad from happening.

Alternatives

  1. Make Ticks public and provide a method to get construct a Mut from Ticks and &mut T. More powerful and more easy to misuse.
  2. Do nothing. People can still do everything they want, but they need to pass (&mut dyn Reflect, impl FnMut() + '_) around instead of Mut<dyn Reflect>

Changelog

  • add MutUntyped::map_unchanged to turn a MutUntyped into its typed counterpart

@github-actions
Copy link
Contributor

Welcome, new contributor!

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

@Selene-Amanita Selene-Amanita added C-Feature A new feature, making something new possible A-ECS Entities, components, systems, and events A-Reflection Runtime information about types A-Pointers Relating to Bevy pointer abstractions C-Usability A targeted quality-of-life change that makes Bevy easier to use and removed C-Feature A new feature, making something new possible labels Jul 19, 2023
Copy link
Member

@JoJoJet JoJoJet left a comment

Choose a reason for hiding this comment

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

Thank you for adopting this!

crates/bevy_ecs/src/change_detection.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/change_detection.rs Outdated Show resolved Hide resolved
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 agree with the doc suggestions but won't block on them.

@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 Jul 21, 2023
Co-authored-by: Joseph <21144246+JoJoJet@users.noreply.github.com>
@Serverator
Copy link
Contributor Author

Serverator commented Jul 21, 2023

Uh huh... For some reason CI is failing now, but I don't really understand why, I only committed the doc suggestions

@JoJoJet
Copy link
Member

JoJoJet commented Jul 21, 2023

One of my suggestions must have had an extra space or something -- you should be able to just run cargo fmt to fix it.

Co-authored-by: Joseph <21144246+JoJoJet@users.noreply.github.com>
@mockersf
Copy link
Member

Miri failure is due to a but in rust nightly, it should be fixed tomorrow

@mockersf mockersf added this pull request to the merge queue Jul 23, 2023
Merged via the queue into bevyengine:main with commit 3b1b60e Jul 23, 2023
@Serverator Serverator deleted the mutuntyped-to-typed branch July 25, 2023 09:46
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 A-Pointers Relating to Bevy pointer abstractions A-Reflection Runtime information about types 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.

6 participants