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] - add a SpatialBundle with visibility and transform components #5344

Closed
wants to merge 6 commits into from

Conversation

mockersf
Copy link
Member

@mockersf mockersf commented Jul 16, 2022

Objective

  • Help user when they need to add both a TransformBundle and a VisibilityBundle

Solution

  • Add a SpatialBundle adding all components

@alice-i-cecile alice-i-cecile added C-Usability A simple quality-of-life change that makes Bevy easier to use A-Transform Translations, rotations and scales A-Hierarchy Parent-child entity hierarchies labels Jul 16, 2022
@alice-i-cecile alice-i-cecile added this to the Bevy 0.8 milestone Jul 16, 2022
@alice-i-cecile
Copy link
Member

@IceSentry this isn't a breaking change but will need to go in the migration guide.

@mockersf
Copy link
Member Author

@alice-i-cecile about your review, I would prefer to keep as is and have a followup PR changing the style everywhere

@alice-i-cecile
Copy link
Member

Alright, I'm on board. Can you make a quick issue so we don't forget?

crates/bevy_render/src/view/visibility/mod.rs Outdated Show resolved Hide resolved
crates/bevy_render/src/base_bundle.rs Outdated Show resolved Hide resolved
/// * To show or hide an entity, you should set its [`Visibility`].
/// * To get the computed visibility of an entity, you should get its [`ComputedVisibility`].
/// * To place or move an entity, you should set its [`Transform`].
/// * To get the global position of an entity, you should get its [`GlobalTransform`].
Copy link
Contributor

Choose a reason for hiding this comment

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

Not just global position, but global transform. :) Maybe note that it is only valid for the current frame after transform propagation has been run?

Copy link
Member

Choose a reason for hiding this comment

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

On the one hand, this is useful information. On the other hand, idk if we should be duplicating it across every bundle that includes GlobalTransform.

Copy link
Member

Choose a reason for hiding this comment

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

I do agree that "global transform" is preferable to "global position".

Copy link
Member Author

Choose a reason for hiding this comment

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

/// * [`Transform`] and [`GlobalTransform`], which describe the position of an entity
///
/// * To show or hide an entity, you should set its [`Visibility`].
/// * To get the computed visibility of an entity, you should get its [`ComputedVisibility`].
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe note that it is only valid after the visibility systems have been run? (check_visibility and visibility_propagate, I think.)

Copy link
Member

Choose a reason for hiding this comment

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

On the one hand, this is useful information. On the other hand, idk if we should be duplicating it across every bundle that includes ComputedVisibility. ComputedVisibility does have this "label dependency" information encoded in the relevant fields already. But it is probably worth also including that in the main ComputedVisibility docs.

Copy link
Member

Choose a reason for hiding this comment

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

(Not something I think we should block on here though)

Copy link
Member Author

Choose a reason for hiding this comment

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

Co-authored-by: Robert Swain <robert.swain@gmail.com>
@mockersf mockersf changed the title add a BaseBundle with visibility and transform components add a SpatialBundle with visibility and transform components Jul 18, 2022
@cart
Copy link
Member

cart commented Jul 18, 2022

bors r+

bors bot pushed a commit that referenced this pull request Jul 18, 2022
# Objective

- Help user when they need to add both a `TransformBundle` and a `VisibilityBundle`

## Solution

- Add a `SpatialBundle` adding all components
@bors bors bot changed the title add a SpatialBundle with visibility and transform components [Merged by Bors] - add a SpatialBundle with visibility and transform components Jul 18, 2022
@bors bors bot closed this Jul 18, 2022
inodentry pushed a commit to IyesGames/bevy that referenced this pull request Aug 8, 2022
…ine#5344)

# Objective

- Help user when they need to add both a `TransformBundle` and a `VisibilityBundle`

## Solution

- Add a `SpatialBundle` adding all components
james7132 pushed a commit to james7132/bevy that referenced this pull request Oct 28, 2022
…ine#5344)

# Objective

- Help user when they need to add both a `TransformBundle` and a `VisibilityBundle`

## Solution

- Add a `SpatialBundle` adding all components
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
…ine#5344)

# Objective

- Help user when they need to add both a `TransformBundle` and a `VisibilityBundle`

## Solution

- Add a `SpatialBundle` adding all components
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-Usability A simple quality-of-life change that makes Bevy easier to use
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

4 participants