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 Mesh documentation #9061

Merged
merged 6 commits into from
Jul 31, 2023

Conversation

Selene-Amanita
Copy link
Member

@Selene-Amanita Selene-Amanita commented Jul 6, 2023

Objective

This PR continues #8885

It aims to improve the Mesh documentation in the following ways:

  • Put everything at the "top level" instead of the "impl".
  • Explain better what is a Mesh, how it can be created, and that it can be edited.
  • Explain it can be used with a Material, and mention StandardMaterial, PbrBundle, ColorMaterial, and ColorMesh2dBundle since those cover most cases
  • Mention the glTF/Bevy vocabulary discrepancy for "Mesh"
  • Add an image for the example
  • Various nitpicky modifications

Note

  • The image I added is 90.3ko which I think is small enough?
  • Since rustdoc doesn't allow cross-reference not in dependencies of a subcrate yet, I have a lot of backtick references that are not links :(
  • Since rustdoc doesn't allow linking to code in the crate (?) I put link to github directly.
  • Since rustdoc doesn't allow embed images in doc yet, maybe soon, I had to put only a link to the image. I don't think it's worth adding embed_doc_image as a dependency for this.

@Selene-Amanita Selene-Amanita added C-Docs An addition or correction to our documentation A-Rendering Drawing game state to the screen labels Jul 6, 2023
Copy link
Contributor

@IceSentry IceSentry left a comment

Choose a reason for hiding this comment

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

Couple of nits. Personally, this feels like it's touching a bit too many things, like I'm not sure the AssetServer or StandardMaterial should really be mentioned here but this really isn't targeted at someone like me so I'm not sure what the best solution is.

crates/bevy_render/src/mesh/mesh/mod.rs Outdated Show resolved Hide resolved
///
/// - UV maps in Bevy are "flipped", `[0.0, 0.0]` = Top-Left (not Bot-Left like `OpenGL`).
Copy link
Contributor

Choose a reason for hiding this comment

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

It's only flipped compared to OpenGL, to me this implies bevy is doing something non-standard here. As far as I know, top-left is part of the gltf spec

In other words, I agree it's worth pointing out, but it should be clearer that it's flipped compared to opengl.

Something like

Suggested change
/// - UV maps in Bevy are "flipped", `[0.0, 0.0]` = Top-Left (not Bot-Left like `OpenGL`).
/// - UV maps in bevy start at the top-left unlike opengl which starts at the bottom-left

crates/bevy_render/src/mesh/mesh/mod.rs Show resolved Hide resolved
///
/// - UV maps in Bevy are "flipped", `[0.0, 0.0]` = Top-Left (not Bot-Left like `OpenGL`).
/// - It is normal for multiple vertices to have the same [position attribute](Mesh::ATTRIBUTE_POSITION) value,
Copy link
Contributor

@IceSentry IceSentry Jul 6, 2023

Choose a reason for hiding this comment

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

Maybe try to rephrase this to not use the word normal? I generally wouldn't mind, but in the context of meshes it might be worth avoiding the potential confusion with vertex normal.

@Selene-Amanita
Copy link
Member Author

Selene-Amanita commented Jul 6, 2023

@IceSentry Thank you for your review!

I tried to fix stuff from your comments in the last commit.
About the "flipped UV" and use of "normal" those weren't my sentences to begin with but I agree it's clearer that way!

Couple of nits. Personally, this feels like it's touching a bit too many things, like I'm not sure the AssetServer or StandardMaterial should really be mentioned here but this really isn't targeted at someone like me so I'm not sure what the best solution is.

I think people might want to use a Mesh in Bevy, have no idea how to, search "Mesh Bevy", and either find this doc, or Bevy's cheatbook, in the first case I think it's good to have some references to them at least.

Also knowing that the "UV" and "Normals" are important to put in a mesh for StandardMaterial to work properly is also important imo.

Tell me if it still needs improvement.

crates/bevy_render/src/mesh/mesh/mod.rs Outdated Show resolved Hide resolved
crates/bevy_render/src/mesh/mesh/mod.rs Outdated Show resolved Hide resolved
crates/bevy_render/src/mesh/mesh/mod.rs Outdated Show resolved Hide resolved
@Selene-Amanita Selene-Amanita 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 Jul 7, 2023
@Selene-Amanita Selene-Amanita modified the milestones: 0.12, 0.11.1 Jul 10, 2023
@Selene-Amanita Selene-Amanita changed the title Improve Mesh Documentation Improve Mesh documentation Jul 12, 2023
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Jul 31, 2023
Merged via the queue into bevyengine:main with commit cbe13f3 Jul 31, 2023
21 checks passed
@Selene-Amanita Selene-Amanita deleted the improve-mesh-doc branch July 31, 2023 19:25
cart pushed a commit that referenced this pull request Aug 10, 2023
# Objective

This PR continues #8885

It aims to improve the `Mesh` documentation in the following ways:
- Put everything at the "top level" instead of the "impl".
- Explain better what is a Mesh, how it can be created, and that it can
be edited.
- Explain it can be used with a `Material`, and mention
`StandardMaterial`, `PbrBundle`, `ColorMaterial`, and
`ColorMesh2dBundle` since those cover most cases
- Mention the glTF/Bevy vocabulary discrepancy for "Mesh"
- Add an image for the example
- Various nitpicky modifications

## Note

- The image I added is 90.3ko which I think is small enough?
- Since rustdoc doesn't allow cross-reference not in dependencies of a
subcrate [yet](rust-lang/rust#74481), I have a
lot of backtick references that are not links :(
- Since rustdoc doesn't allow linking to code in the crate (?) I put
link to github directly.
- Since rustdoc doesn't allow embed images in doc
[yet](rust-lang/rust#32104), maybe
[soon](rust-lang/rfcs#3397), I had to put only a
link to the image. I don't think it's worth adding
[embed_doc_image](https://docs.rs/embed-doc-image/latest/embed_doc_image/)
as a dependency for this.
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