add `MutUntyped::map_unchanged` by jakobhellermann · Pull Request #6430 · bevyengine/bevy

jakobhellermann · 2022-10-31T12:24:26Z

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

Make Ticks public and provide a method to get construct a Mut from Ticks and &mut T. More powerful and more easy to misuse.
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

alice-i-cecile · 2022-10-31T15:46:26Z

Definitely opposed to alternate solution 1: it exposes far more internals of change detection than I'd like. I trust you that solution 2 is too annoying.

alice-i-cecile

More pointer shenanigans. It makes sense why this is needed, and the docs are great. Very very grateful for the lifetimed pointer types here.

We can't check whether the types match, because we literally don't store that information on the MutUntyped :(

I'm on board.

joseph-gio

Seems very useful. This will probably simplify some internal code for trait queries.

joseph-gio

A few more things, but LGTM. The PR title and description need to be updated.

alice-i-cecile · 2023-01-01T02:17:14Z

I'd like to see PR description and title updated + the nit addressed before we merge.

# Objective `MutUntyped` is a struct that stores a `PtrMut` alongside change tick metadata. Working with this type is cumbersome, and has few benefits over storing the pointer and change ticks separately. Related: #6430 (title is out of date) ## Solution Add a convenience method for transforming an untyped change detection pointer into its typed counterpart. --- ## Changelog - Added the method `MutUntyped::with_type`.

# Objective `MutUntyped` is a struct that stores a `PtrMut` alongside change tick metadata. Working with this type is cumbersome, and has few benefits over storing the pointer and change ticks separately. Related: bevyengine#6430 (title is out of date) ## Solution Add a convenience method for transforming an untyped change detection pointer into its typed counterpart. --- ## Changelog - Added the method `MutUntyped::with_type`.

jakobhellermann · 2023-02-13T20:51:23Z

I'd like to see PR description and title updated + the nit addressed before we merge.

Done

alice-i-cecile · 2023-02-13T21:06:18Z

@jakobhellermann once CI is passing I'm comfortable merging this.

jakobhellermann · 2023-02-17T11:25:26Z

CI is green

alice-i-cecile · 2023-02-17T15:20:28Z

@jakobhellermann if you could remove that unsafe here in the next day or two that would be great. I'll be sure to get this in regardless though; we can make a quick follow-up PR.

alice-i-cecile · 2023-02-17T15:22:38Z

bors r+

# 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: ```rust // 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 ```rust 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

bors · 2023-02-17T15:35:34Z

Build failed (retrying...):

check-compiles

# 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: ```rust // 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 ```rust 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

bors · 2023-02-17T15:37:29Z

Build failed:

build (macos-latest)

BoxyUwU · 2023-02-26T17:02:06Z

Your method also only works if you a type T to use?

jakobhellermann · 2023-02-26T17:04:19Z

If I have a ReflectFromPtr instance I can use that to map the MutUntyped to a Mut<dyn Reflect>. Theoretically, not without this PR.

No ::<T> involved.

BoxyUwU · 2023-02-26T17:18:58Z

ah I see what you mean, with this you can do with_type like stuff that requires more than just .deref/.deref_mut

Co-authored-by: JoJoJet <21144246+JoJoJet@users.noreply.github.com>

jakobhellermann · 2023-05-28T12:23:31Z

rebased. Are there any requested changes left? I think they're all addressed

alice-i-cecile

This appears complete now, and the PR description looks good.

joseph-gio · 2023-05-28T21:10:43Z

Looks like CI is failing due to #7905, which renamed the last_change_tick and change_tick fields to last_run and this_run. Also, I don't think the Complex tag is necessary since the only thing this function does is call a closure.

alice-i-cecile · 2023-05-29T15:44:52Z

@jakobhellermann once this is passing CI I'll merge it in :)

alice-i-cecile · 2023-06-05T20:15:13Z

Adding Adopt-Me on this since it's been a week <3 Shouldn't be hard to clean up at all.

joseph-gio · 2023-07-21T19:00:51Z

Closing in favor of #9194, which is an adoption of this PR :)

### **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: ```rust // 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 ```rust 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 --------- Co-authored-by: Jakob Hellermann <jakob.hellermann@protonmail.com> Co-authored-by: JoJoJet <21144246+JoJoJet@users.noreply.github.com>

jakobhellermann added C-Usability A targeted quality-of-life change that makes Bevy easier to use A-Reflection Runtime information about types A-ECS Entities, components, systems, and events labels Oct 31, 2022

alice-i-cecile added the D-Complex Quite challenging from either a design or technical perspective. Ask for help! label Oct 31, 2022

alice-i-cecile approved these changes Oct 31, 2022

View reviewed changes

james7132 requested changes Dec 17, 2022

View reviewed changes

Comment thread crates/bevy_ecs/src/change_detection.rs Outdated

Comment thread crates/bevy_ecs/src/change_detection.rs

joseph-gio suggested changes Dec 21, 2022

View reviewed changes

Comment thread crates/bevy_ecs/src/change_detection.rs Outdated

jakobhellermann force-pushed the mutuntyped-to-typed branch from 862e285 to ceb2fd7 Compare December 31, 2022 20:38

joseph-gio approved these changes Dec 31, 2022

View reviewed changes

Comment thread crates/bevy_ecs/src/change_detection.rs Outdated

joseph-gio mentioned this pull request Jan 6, 2023

[Merged by Bors] - Add a method for converting MutUntyped -> Mut<T> #7113

Closed

jakobhellermann changed the title ~~add MutUntyped::to_typed~~ add MutUntyped::map_unchanged Feb 13, 2023

jakobhellermann force-pushed the mutuntyped-to-typed branch from e283c43 to caa7122 Compare February 13, 2023 20:51

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 Feb 13, 2023

joseph-gio approved these changes Feb 17, 2023

View reviewed changes

Comment thread crates/bevy_ecs/src/change_detection.rs Outdated

joseph-gio reviewed Feb 17, 2023

View reviewed changes

Comment thread crates/bevy_ecs/src/change_detection.rs

alice-i-cecile added this to the 0.10 milestone Feb 17, 2023

joseph-gio reviewed Mar 6, 2023

View reviewed changes

Comment thread crates/bevy_ecs/src/change_detection.rs Outdated

jakobhellermann force-pushed the mutuntyped-to-typed branch from 2d339c4 to 3c0c068 Compare March 6, 2023 19:23

jakobhellermann and others added 9 commits May 28, 2023 14:22

add MutUntyped::to_typed

f9ce9f5

rename to_typed to map_unchanged

e422ad4

docs tweaks

291694b

remove merge conflict marker

f7a9319

fix docs

7c5fb10

fix test

aa8112c

merge imports

c1d07dc

Update crates/bevy_ecs/src/change_detection.rs

645ba50

Co-authored-by: JoJoJet <21144246+JoJoJet@users.noreply.github.com>

remove unused import

251825a

jakobhellermann force-pushed the mutuntyped-to-typed branch from 3c0c068 to 251825a Compare May 28, 2023 12:23

alice-i-cecile approved these changes May 28, 2023

View reviewed changes

alice-i-cecile requested review from james7132 and joseph-gio May 28, 2023 14:25

joseph-gio approved these changes May 28, 2023

View reviewed changes

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 May 28, 2023

joseph-gio removed the D-Complex Quite challenging from either a design or technical perspective. Ask for help! label May 28, 2023

alice-i-cecile added the S-Adopt-Me The original PR author has no intent to complete this work. Pick me up! label Jun 5, 2023

Serverator mentioned this pull request Jul 18, 2023

add MutUntyped::map_unchanged #9194

Merged

joseph-gio closed this Jul 21, 2023

Uh oh!

Uh oh!

Conversation

jakobhellermann commented Oct 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Objective

Solution

Alternatives

Changelog

Uh oh!

alice-i-cecile commented Oct 31, 2022

Uh oh!

alice-i-cecile left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

joseph-gio left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

joseph-gio left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alice-i-cecile commented Jan 1, 2023

Uh oh!

jakobhellermann commented Feb 13, 2023

Uh oh!

alice-i-cecile commented Feb 13, 2023

Uh oh!

jakobhellermann commented Feb 17, 2023

Uh oh!

Uh oh!

Uh oh!

alice-i-cecile commented Feb 17, 2023

Uh oh!

alice-i-cecile commented Feb 17, 2023

Uh oh!

bors Bot commented Feb 17, 2023

Uh oh!

bors Bot commented Feb 17, 2023

Uh oh!

BoxyUwU commented Feb 26, 2023

Uh oh!

jakobhellermann commented Feb 26, 2023

Uh oh!

BoxyUwU commented Feb 26, 2023

Uh oh!

Uh oh!

jakobhellermann commented May 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alice-i-cecile left a comment

Choose a reason for hiding this comment

Uh oh!

joseph-gio commented May 28, 2023

Uh oh!

alice-i-cecile commented May 29, 2023

Uh oh!

alice-i-cecile commented Jun 5, 2023

Uh oh!

joseph-gio commented Jul 21, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jakobhellermann commented Oct 31, 2022 •

edited

Loading

jakobhellermann commented May 28, 2023 •

edited

Loading