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

Improve SpatialBundle docs #9673

Merged
merged 3 commits into from
Sep 3, 2023

Conversation

Nilirad
Copy link
Contributor

@Nilirad Nilirad commented Sep 2, 2023

Objective

This PR aims to fix a handful of problems with the SpatialBundle docs:

The docs describe the role of the single components of the bundle, overshadowing the purpose of SpatialBundle itself. Also, those items may be added, removed or changed over time, as it happened with #9497, requiring a higher maintenance effort, which will often result in errors, as it happened.

Solution

Just describe the role of SpatialBundle and of the transform and visibility concepts, without mentioning the specific component types. Since the bundle has public fields, the reader can easily click them and read the documentation if they need to know more. I removed the mention of numbers of components since they were four, now they are five, and who knows how many they will be in the future. In this process, I removed the bullet points, which are no longer needed, and were contextually wrong in the first place, since they were meant to list the components, but ended up describing use-cases and requirements for hierarchies.

@Nilirad Nilirad added the C-Docs An addition or correction to our documentation label Sep 2, 2023
@alice-i-cecile alice-i-cecile added the A-Rendering Drawing game state to the screen label Sep 2, 2023
/// * To get the global transform of an entity, you should get its [`GlobalTransform`].
/// * For hierarchies to work correctly, you must have all four components.
/// * You may use the [`SpatialBundle`] to guarantee this.
/// It consists of transform components,
Copy link
Member

Choose a reason for hiding this comment

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

I think this whole paragraph is unhelpful and prone to getting outdated.

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 intent here, but I think we can clean it up further.

@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 Sep 2, 2023
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Sep 2, 2023
Merged via the queue into bevyengine:main with commit 532f3cb Sep 3, 2023
22 checks passed
@bzm3r
Copy link
Contributor

bzm3r commented Sep 3, 2023

Something that is a little confusing to me: why is visibility part of the SpatialBundle? That's the only part of the doc explanation that makes me scratch my head: position, rotation, scale and visibility...one of those is not quite like the others.

It may also be worthwhile to leave a tiny bit of info about what kind of scale scale is (e.g. is it a "uniform scale in all directions"? or a scale defined by N numbers, one for each axis? or a scale defined by N numbers, for a coordinate system relative to the object?). Or is this also likely to get outdated?

@Nilirad
Copy link
Contributor Author

Nilirad commented Sep 3, 2023

why is visibility part of the SpatialBundle? That's the only part of the doc explanation that makes me scratch my head: position, rotation, scale and visibility...one of those is not quite like the others.

I believe it's because the purpose this bundle is actually for rendering. Therefore, visibility has a fundamental role in answering the question "where should this be drawn?"

It may also be worthwhile to leave a tiny bit of info about what kind of scale scale is

That detail would be better suited for Transform.scale

rdrpenguin04 pushed a commit to rdrpenguin04/bevy that referenced this pull request Jan 9, 2024
# Objective

This PR aims to fix a handful of problems with the `SpatialBundle` docs:

The docs describe the role of the single components of the bundle,
overshadowing the purpose of `SpatialBundle` itself. Also, those items
may be added, removed or changed over time, as it happened with bevyengine#9497,
requiring a higher maintenance effort, which will often result in
errors, as it happened.

## Solution

Just describe the role of `SpatialBundle` and of the transform and
visibility concepts, without mentioning the specific component types.
Since the bundle has public fields, the reader can easily click them and
read the documentation if they need to know more. I removed the mention
of numbers of components since they were four, now they are five, and
who knows how many they will be in the future. In this process, I
removed the bullet points, which are no longer needed, and were
contextually wrong in the first place, since they were meant to list the
components, but ended up describing use-cases and requirements for
hierarchies.

---------

Co-authored-by: Alice Cecile <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Rendering Drawing game state to the screen C-Docs An addition or correction to our documentation 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.

4 participants