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] - Remove the GlobalTransform::translation_mut method #7134

Closed
wants to merge 2 commits into from

Conversation

nicopap
Copy link
Contributor

@nicopap nicopap commented Jan 8, 2023

Objective

It is possible to manually update GlobalTransform.
The engine actually assumes this is not possible.
For example, propagate_transform does not update children
of an Entity which GlobalTransform changed,
leading to unexpected behaviors.

A GlobalTransform set by the user may also be blindly
overwritten by the propagation system.

Solution

  • Remove translation_mut
  • Explain to users that they shouldn't manually update the GlobalTransform
  • Remove global_vs_local.rs example, since it misleads users
    in believing that it is a valid use-case to manually update the
    GlobalTransform

Changelog

  • Remove GlobalTransform::translation_mut

Migration Guide

GlobalTransform::translation_mut has been removed without alternative,
if you were relying on this, update the Transform instead. If the given entity
had children or parent, you may need to remove its parent to make its transform
independent (in which case the new Commands::set_parent_in_place and
Commands::remove_parent_in_place may be of interest)

Bevy may add in the future a way to toggle transform propagation on
an entity basis.

@nicopap nicopap added C-Bug An unexpected or incorrect behavior A-Transform Translations, rotations and scales M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide A-Hierarchy Parent-child entity hierarchies labels Jan 8, 2023
@DasLixou
Copy link
Contributor

DasLixou commented Jan 8, 2023

I would rather like making the engine understand changing the GlobalTransform is possible

@nicopap nicopap added the X-Controversial There is active debate or serious implications around merging this PR label Jan 8, 2023
@mockersf
Copy link
Member

mockersf commented Jan 8, 2023

It seems there is only mutable access to the translation of the global transform, which makes it not that useful...

So I'm OK with removing it

@james7132 james7132 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 Jan 9, 2023
@nicopap
Copy link
Contributor Author

nicopap commented Jan 9, 2023

Another important thing to note is that being able to mutate the GlobalTransform could be real useful in the case of a custom transform system. Of course this is impossible to do with the current API. I might just open an issue if this gets merged.

@aevyrie
Copy link
Member

aevyrie commented Jan 9, 2023

I've written a couple custom transform propagation systems; I don't believe this would prevent that as long as GlobalTransforms fields are pub. My strategy has been to disable bevy's built-in systems, and provide my own drop-in replacements. I agree that this change will improve the GlobalTransform API.

@alice-i-cecile alice-i-cecile added this to the 0.10 milestone Jan 10, 2023
@alice-i-cecile alice-i-cecile removed the X-Controversial There is active debate or serious implications around merging this PR label Jan 10, 2023
@alice-i-cecile
Copy link
Member

bors r+

bors bot pushed a commit that referenced this pull request Jan 10, 2023
# Objective

It is possible to manually update `GlobalTransform`.
The engine actually assumes this is not possible.
For example, `propagate_transform` does not update children
of an `Entity` which **`GlobalTransform`** changed,
leading to unexpected behaviors.

A `GlobalTransform` set by the user may also be blindly
overwritten by the propagation system.

## Solution

- Remove `translation_mut`
- Explain to users that they shouldn't manually update the `GlobalTransform`
- Remove `global_vs_local.rs` example, since it misleads users
  in believing that it is a valid use-case to manually update the
  `GlobalTransform`

---

## Changelog

- Remove `GlobalTransform::translation_mut`

## Migration Guide

`GlobalTransform::translation_mut` has been removed without alternative,
if you were relying on this, update the `Transform` instead. If the given entity
had children or parent, you may need to remove its parent to make its transform
independent (in which case the new `Commands::set_parent_in_place` and
`Commands::remove_parent_in_place` may be of interest)

Bevy may add in the future a way to toggle transform propagation on
an entity basis.
@bors
Copy link
Contributor

bors bot commented Jan 10, 2023

Build failed (retrying...):

bors bot pushed a commit that referenced this pull request Jan 10, 2023
# Objective

It is possible to manually update `GlobalTransform`.
The engine actually assumes this is not possible.
For example, `propagate_transform` does not update children
of an `Entity` which **`GlobalTransform`** changed,
leading to unexpected behaviors.

A `GlobalTransform` set by the user may also be blindly
overwritten by the propagation system.

## Solution

- Remove `translation_mut`
- Explain to users that they shouldn't manually update the `GlobalTransform`
- Remove `global_vs_local.rs` example, since it misleads users
  in believing that it is a valid use-case to manually update the
  `GlobalTransform`

---

## Changelog

- Remove `GlobalTransform::translation_mut`

## Migration Guide

`GlobalTransform::translation_mut` has been removed without alternative,
if you were relying on this, update the `Transform` instead. If the given entity
had children or parent, you may need to remove its parent to make its transform
independent (in which case the new `Commands::set_parent_in_place` and
`Commands::remove_parent_in_place` may be of interest)

Bevy may add in the future a way to toggle transform propagation on
an entity basis.
@bors
Copy link
Contributor

bors bot commented Jan 10, 2023

Build failed (retrying...):

@alice-i-cecile
Copy link
Member

bors retry

@bors
Copy link
Contributor

bors bot commented Jan 10, 2023

Already running a review

bors bot pushed a commit that referenced this pull request Jan 10, 2023
# Objective

It is possible to manually update `GlobalTransform`.
The engine actually assumes this is not possible.
For example, `propagate_transform` does not update children
of an `Entity` which **`GlobalTransform`** changed,
leading to unexpected behaviors.

A `GlobalTransform` set by the user may also be blindly
overwritten by the propagation system.

## Solution

- Remove `translation_mut`
- Explain to users that they shouldn't manually update the `GlobalTransform`
- Remove `global_vs_local.rs` example, since it misleads users
  in believing that it is a valid use-case to manually update the
  `GlobalTransform`

---

## Changelog

- Remove `GlobalTransform::translation_mut`

## Migration Guide

`GlobalTransform::translation_mut` has been removed without alternative,
if you were relying on this, update the `Transform` instead. If the given entity
had children or parent, you may need to remove its parent to make its transform
independent (in which case the new `Commands::set_parent_in_place` and
`Commands::remove_parent_in_place` may be of interest)

Bevy may add in the future a way to toggle transform propagation on
an entity basis.
@bors bors bot changed the title Remove the GlobalTransform::translation_mut method [Merged by Bors] - Remove the GlobalTransform::translation_mut method Jan 10, 2023
@bors bors bot closed this Jan 10, 2023
alradish pushed a commit to alradish/bevy that referenced this pull request Jan 22, 2023
# Objective

It is possible to manually update `GlobalTransform`.
The engine actually assumes this is not possible.
For example, `propagate_transform` does not update children
of an `Entity` which **`GlobalTransform`** changed,
leading to unexpected behaviors.

A `GlobalTransform` set by the user may also be blindly
overwritten by the propagation system.

## Solution

- Remove `translation_mut`
- Explain to users that they shouldn't manually update the `GlobalTransform`
- Remove `global_vs_local.rs` example, since it misleads users
  in believing that it is a valid use-case to manually update the
  `GlobalTransform`

---

## Changelog

- Remove `GlobalTransform::translation_mut`

## Migration Guide

`GlobalTransform::translation_mut` has been removed without alternative,
if you were relying on this, update the `Transform` instead. If the given entity
had children or parent, you may need to remove its parent to make its transform
independent (in which case the new `Commands::set_parent_in_place` and
`Commands::remove_parent_in_place` may be of interest)

Bevy may add in the future a way to toggle transform propagation on
an entity basis.
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
# Objective

It is possible to manually update `GlobalTransform`.
The engine actually assumes this is not possible.
For example, `propagate_transform` does not update children
of an `Entity` which **`GlobalTransform`** changed,
leading to unexpected behaviors.

A `GlobalTransform` set by the user may also be blindly
overwritten by the propagation system.

## Solution

- Remove `translation_mut`
- Explain to users that they shouldn't manually update the `GlobalTransform`
- Remove `global_vs_local.rs` example, since it misleads users
  in believing that it is a valid use-case to manually update the
  `GlobalTransform`

---

## Changelog

- Remove `GlobalTransform::translation_mut`

## Migration Guide

`GlobalTransform::translation_mut` has been removed without alternative,
if you were relying on this, update the `Transform` instead. If the given entity
had children or parent, you may need to remove its parent to make its transform
independent (in which case the new `Commands::set_parent_in_place` and
`Commands::remove_parent_in_place` may be of interest)

Bevy may add in the future a way to toggle transform propagation on
an entity basis.
@nicopap nicopap deleted the remove-mut-global branch August 30, 2023 13:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Hierarchy Parent-child entity hierarchies A-Transform Translations, rotations and scales C-Bug An unexpected or incorrect behavior M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide 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