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] - Round out the untyped api s #7009

Closed

Conversation

TheRawMeatball
Copy link
Member

@TheRawMeatball TheRawMeatball commented Dec 22, 2022

Objective

Bevy uses custom Ptr types so the rust borrow checker can help ensure lifetimes are correct, even when types aren't known. However, these types don't benefit from the automatic lifetime coercion regular rust references enjoy

Solution

Add a couple methods to Ptr, PtrMut, and MutUntyped to allow for easy usage of these types in more complex scenarios.

Changelog

  • Added as_mut and as_ref methods to MutUntyped.
  • Added shrink and as_ref methods to PtrMut.

Migration Guide

  • MutUntyped::into_inner now marks things as changed.

///
/// In order to mark the value as changed, you need to call [`set_changed`](DetectChanges::set_changed) manually.
#[inline]
pub fn as_mut(&mut self) -> PtrMut<'_> {
Copy link
Member

@JoJoJet JoJoJet Dec 22, 2022

Choose a reason for hiding this comment

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

I don't like how this silently bypasses change detection, it kind of defeats the point of having a change detection wrapper type. I see that MutUntyped::into_inner already does this, but I think we should fix that too.

For consistency with Mut, anything that might mutate the inner value should mark a change, and there should be an explicit bypass_change_detection method for getting around that.

Copy link
Member

Choose a reason for hiding this comment

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

Also, this change should go in the migration guide.


/// Gets a `PtrMut` from this with a smaller lifetime
#[inline]
pub fn shrink(&mut self) -> PtrMut<'_> {
Copy link
Member

@JoJoJet JoJoJet Dec 22, 2022

Choose a reason for hiding this comment

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

If this method is chained together as part of a complex expression, I feel like it would be very unclear what this method is actually doing. reborrow would be a better name IMO.

@james7132 james7132 added the C-Usability A simple quality-of-life change that makes Bevy easier to use label Dec 23, 2022
crates/bevy_ptr/src/lib.rs Outdated Show resolved Hide resolved
crates/bevy_ptr/src/lib.rs Outdated Show resolved Hide resolved
crates/bevy_ptr/src/lib.rs Outdated Show resolved Hide resolved

/// Gets an mutable reference from this
#[inline]
pub fn as_mut(&mut self) -> PtrMut<'_> {
Copy link
Member

Choose a reason for hiding this comment

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

Might make more sense to call this downgrade, or something else to match PtrMut::promote.

Copy link
Member

Choose a reason for hiding this comment

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

demote is the natural counterpart.

Copy link
Member

Choose a reason for hiding this comment

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

On second thought, calling this demote or downgrade would only make sense if the method took self by value.

@alice-i-cecile alice-i-cecile added the A-ECS Entities, components, systems, and events label Dec 27, 2022
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 would like to see:

  1. The doc / naming nits are addressed.
  2. A bypass_change_detection method.

@mockersf mockersf added the C-Breaking-Change A breaking change to Bevy's public API that needs to be noted in a migration guide label Dec 27, 2022
@mockersf
Copy link
Member

Marking as breaking change as into_inner now trigger change detection. While I agree with the change, it needs to be documented in the changelog and ideally migration guide

Co-authored-by: JoJoJet <21144246+JoJoJet@users.noreply.github.com>
@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 Dec 27, 2022
@alice-i-cecile
Copy link
Member

bors r+

bors bot pushed a commit that referenced this pull request Dec 27, 2022
# Objective

Bevy uses custom `Ptr` types so the rust borrow checker can help ensure lifetimes are correct, even when types aren't known. However, these types don't benefit from the automatic lifetime coercion regular rust references enjoy

## Solution

Add a couple methods to Ptr, PtrMut, and MutUntyped to allow for easy usage of these types in more complex scenarios.

## Changelog

- Added `as_mut` and `as_ref` methods to `MutUntyped`.
- Added `shrink` and `as_ref` methods to `PtrMut`.

## Migration Guide

- `MutUntyped::into_inner` now marks things as changed.
@bors bors bot changed the title Round out the untyped api s [Merged by Bors] - Round out the untyped api s Dec 27, 2022
@bors bors bot closed this Dec 27, 2022
alradish pushed a commit to alradish/bevy that referenced this pull request Jan 22, 2023
# Objective

Bevy uses custom `Ptr` types so the rust borrow checker can help ensure lifetimes are correct, even when types aren't known. However, these types don't benefit from the automatic lifetime coercion regular rust references enjoy

## Solution

Add a couple methods to Ptr, PtrMut, and MutUntyped to allow for easy usage of these types in more complex scenarios.

## Changelog

- Added `as_mut` and `as_ref` methods to `MutUntyped`.
- Added `shrink` and `as_ref` methods to `PtrMut`.

## Migration Guide

- `MutUntyped::into_inner` now marks things as changed.
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
# Objective

Bevy uses custom `Ptr` types so the rust borrow checker can help ensure lifetimes are correct, even when types aren't known. However, these types don't benefit from the automatic lifetime coercion regular rust references enjoy

## Solution

Add a couple methods to Ptr, PtrMut, and MutUntyped to allow for easy usage of these types in more complex scenarios.

## Changelog

- Added `as_mut` and `as_ref` methods to `MutUntyped`.
- Added `shrink` and `as_ref` methods to `PtrMut`.

## Migration Guide

- `MutUntyped::into_inner` now marks things as changed.
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-Breaking-Change A breaking change to Bevy's public API that needs to be noted in a migration guide C-Usability A simple 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.

5 participants