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 3d shapes example #4613

Closed
wants to merge 8 commits into from

Conversation

rparrett
Copy link
Contributor

@rparrett rparrett commented Apr 27, 2022

Objective

Solution

3d_shapes_proc_sm.mp4
  • Add an example showcasing the built-in 3d shapes with lighting/shadows
  • Rotate objects in such a way that all faces are seen by the camera
  • Add a UV debug texture

Discussion

I'm not sure if this is what @alice-i-cecile had in mind, but I adapted the little "torus playground" from the issue linked above to include all built-in shapes.

This exact arrangement might not be particularly scalable if many more shapes are added. Maybe a slow camera pan, or cycling with the keyboard or on a timer, or a sidebar with buttons would work better. If one of the latter options is used, options for showing wireframes or computed flat normals might add some additional utility.

Ideally, I think we'd have a better way of visualizing normals.

Happy to rework this or close it if there's not a consensus around it being useful.

@github-actions github-actions bot added the S-Needs-Triage This issue needs to be labelled label Apr 27, 2022
@DJMcNab
Copy link
Member

DJMcNab commented Apr 27, 2022

Is it possible/feasible to generate the uv_debug texture programmatically?

151kB isn't bad, but it's best not to add more 'large textures' to the git history if we can avoid it.

That being said, we have all of https://github.com/bevyengine/bevy/tree/328c26d02c50de0bc77f0d24a376f43ba89517b1/assets/models/FlightHelmet, so maybe that ship has sailed?

@rparrett
Copy link
Contributor Author

rparrett commented Apr 27, 2022

Is it possible/feasible to generate the uv_debug texture programmatically?

151kB isn't bad, but it's best not to add more 'large textures' to the git history if we can avoid it.

That being said, we have all of https://github.com/bevyengine/bevy/tree/328c26d02c50de0bc77f0d24a376f43ba89517b1/assets/models/FlightHelmet, so maybe that ship has sailed?

Good point. I processed the texture with tinypng and it ended up at 39k. But I'd also prefer not to add textures to the repo. At the very least, a 512x512 version would probably be just as functional.

@mockersf mockersf added C-Examples An addition or correction to our examples and removed S-Needs-Triage This issue needs to be labelled labels Apr 27, 2022
@alice-i-cecile
Copy link
Member

alice-i-cecile commented Apr 27, 2022

This is exactly what I had in mind, thank you! I would also prefer if we can generate the textures programmatically, but if it's only a single reused texture it's not as bad as I was initially concerned.

This is also quite nice as a learning example: it's simple, idiomatic code that showcases the different shapes and basics of 3D.

@alice-i-cecile alice-i-cecile added the A-Rendering Drawing game state to the screen label Apr 27, 2022
examples/README.md Outdated Show resolved Hide resolved
@alice-i-cecile
Copy link
Member

Could you update the PR description with a fresh clip of the example with the new texture?

@rparrett
Copy link
Contributor Author

I replaced the texture with generated pattern that's equivalent to the previous texture without the text, arrows, and small grid.

Could you update the PR description with a fresh clip of the example with the new texture?

Done.

@Nilirad
Copy link
Contributor

Nilirad commented Apr 27, 2022

It would be actually useful to have a in-engine, procedurally generated placeholder texture for 3D meshes. Many engines do that for missing textures.

@alice-i-cecile
Copy link
Member

It would be actually useful to have a in-engine, procedurally generated placeholder texture for 3D meshes. Many engines do that for missing textures.

Agreed. Maybe we should move your texture snippet into one of our rendering crates, and then just use the public API?

Copy link
Contributor

@Nilirad Nilirad left a comment

Choose a reason for hiding this comment

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

Some opinions:

  1. I think it's better to rename the example to 3d_shapes, since in future we'll probably also have an example for 2D shapes.
  2. It should have a doc comment at the top briefly describing the example as specified in Module-style doc strings for examples #3951
  3. A doc comment specifying the role of the Shape component wouldn't be bad.

examples/README.md Outdated Show resolved Hide resolved
examples/3d/shapes.rs Outdated Show resolved Hide resolved
@rparrett
Copy link
Contributor Author

rparrett commented Apr 28, 2022

Agreed. Maybe we should move your texture snippet into one of our rendering crates, and then just use the public API?

I'm a bit hesitant to push this PR that far into "non-triviality" but I'd be pretty excited to work on procedural debug textures with some guidance from folks who have a better idea of

  • Where that code should live
  • What sorts of simple texture(s) would actually be useful

I think it's better to rename the example to 3d_shapes, since in future we'll probably also have an example for 2D shapes.

Sounds good to me if others are on board.

@alice-i-cecile
Copy link
Member

I like the example rename for sure :)

I think we should move forward with the current approach to texturing now, and then create a new issue for debug textures, noting that we should migrate the texture found in this example to there.

@Nilirad Nilirad mentioned this pull request May 1, 2022
Copy link
Contributor

@Nilirad Nilirad left a comment

Choose a reason for hiding this comment

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

Just an optional nitpick on the filename. In any case, I approve.

@@ -200,6 +200,10 @@ path = "examples/3d/pbr.rs"
name = "shadow_biases"
path = "examples/3d/shadow_biases.rs"

[[example]]
name = "3d_shapes"
path = "examples/3d/shapes.rs"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
path = "examples/3d/shapes.rs"
path = "examples/3d/3d_shapes.rs"

The problem with this is that if a user sees in the file tree shapes.rs thinks that the example name is shapes.

Remember to rename the file and change the reference to the README if you accept this.

@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 May 1, 2022
@alice-i-cecile
Copy link
Member

bors r+

bors bot pushed a commit that referenced this pull request May 2, 2022
# Objective

- As requested here: #4520 (comment)
- Make it easier to spot issues with built-in shapes

## Solution

https://user-images.githubusercontent.com/200550/165624709-c40dfe7e-0e1e-4bd3-ae52-8ae66888c171.mp4

- Add an example showcasing the built-in 3d shapes with lighting/shadows
- Rotate objects in such a way that all faces are seen by the camera
- Add a UV debug texture

## Discussion

I'm not sure if this is what @alice-i-cecile had in mind, but I adapted the little "torus playground" from the issue linked above to include all built-in shapes.

This exact arrangement might not be particularly scalable if many more shapes are added. Maybe a slow camera pan, or cycling with the keyboard or on a timer, or a sidebar with buttons would work better. If one of the latter options is used, options for showing wireframes or computed flat normals might add some additional utility.

Ideally, I think we'd have a better way of visualizing normals.

Happy to rework this or close it if there's not a consensus around it being useful.
@bors
Copy link
Contributor

bors bot commented May 2, 2022

This PR was included in a batch that was canceled, it will be automatically retried

bors bot pushed a commit that referenced this pull request May 2, 2022
# Objective

- As requested here: #4520 (comment)
- Make it easier to spot issues with built-in shapes

## Solution

https://user-images.githubusercontent.com/200550/165624709-c40dfe7e-0e1e-4bd3-ae52-8ae66888c171.mp4

- Add an example showcasing the built-in 3d shapes with lighting/shadows
- Rotate objects in such a way that all faces are seen by the camera
- Add a UV debug texture

## Discussion

I'm not sure if this is what @alice-i-cecile had in mind, but I adapted the little "torus playground" from the issue linked above to include all built-in shapes.

This exact arrangement might not be particularly scalable if many more shapes are added. Maybe a slow camera pan, or cycling with the keyboard or on a timer, or a sidebar with buttons would work better. If one of the latter options is used, options for showing wireframes or computed flat normals might add some additional utility.

Ideally, I think we'd have a better way of visualizing normals.

Happy to rework this or close it if there's not a consensus around it being useful.
@bors
Copy link
Contributor

bors bot commented May 2, 2022

Build failed (retrying...):

bors bot pushed a commit that referenced this pull request May 2, 2022
# Objective

- As requested here: #4520 (comment)
- Make it easier to spot issues with built-in shapes

## Solution

https://user-images.githubusercontent.com/200550/165624709-c40dfe7e-0e1e-4bd3-ae52-8ae66888c171.mp4

- Add an example showcasing the built-in 3d shapes with lighting/shadows
- Rotate objects in such a way that all faces are seen by the camera
- Add a UV debug texture

## Discussion

I'm not sure if this is what @alice-i-cecile had in mind, but I adapted the little "torus playground" from the issue linked above to include all built-in shapes.

This exact arrangement might not be particularly scalable if many more shapes are added. Maybe a slow camera pan, or cycling with the keyboard or on a timer, or a sidebar with buttons would work better. If one of the latter options is used, options for showing wireframes or computed flat normals might add some additional utility.

Ideally, I think we'd have a better way of visualizing normals.

Happy to rework this or close it if there's not a consensus around it being useful.
@bors bors bot changed the title Add 3d shapes example [Merged by Bors] - Add 3d shapes example May 2, 2022
@bors bors bot closed this May 2, 2022
exjam pushed a commit to exjam/bevy that referenced this pull request May 22, 2022
# Objective

- As requested here: bevyengine#4520 (comment)
- Make it easier to spot issues with built-in shapes

## Solution

https://user-images.githubusercontent.com/200550/165624709-c40dfe7e-0e1e-4bd3-ae52-8ae66888c171.mp4

- Add an example showcasing the built-in 3d shapes with lighting/shadows
- Rotate objects in such a way that all faces are seen by the camera
- Add a UV debug texture

## Discussion

I'm not sure if this is what @alice-i-cecile had in mind, but I adapted the little "torus playground" from the issue linked above to include all built-in shapes.

This exact arrangement might not be particularly scalable if many more shapes are added. Maybe a slow camera pan, or cycling with the keyboard or on a timer, or a sidebar with buttons would work better. If one of the latter options is used, options for showing wireframes or computed flat normals might add some additional utility.

Ideally, I think we'd have a better way of visualizing normals.

Happy to rework this or close it if there's not a consensus around it being useful.
bors bot pushed a commit that referenced this pull request Sep 24, 2022
# Objective

I was about to submit a PR to add these two examples to `bevy-website` and re-discovered the inconsistency.

Although it's not a major issue on the website where only the filenames are shown, this would help to visually distinguish the two examples in the list  because the names are very prominent.

This also helps out when fuzzy-searching the codebase for these files.

## Solution

Rename `shapes` to `2d_shapes`. Now the filename matches the example name, and the naming structure matches the 3d example.

## Notes

@Nilirad proposed this in #4613 (comment) but it had slipped away from my brain at that time.
bors bot pushed a commit that referenced this pull request Sep 24, 2022
# Objective

I was about to submit a PR to add these two examples to `bevy-website` and re-discovered the inconsistency.

Although it's not a major issue on the website where only the filenames are shown, this would help to visually distinguish the two examples in the list  because the names are very prominent.

This also helps out when fuzzy-searching the codebase for these files.

## Solution

Rename `shapes` to `2d_shapes`. Now the filename matches the example name, and the naming structure matches the 3d example.

## Notes

@Nilirad proposed this in #4613 (comment) but it had slipped away from my brain at that time.
bors bot pushed a commit that referenced this pull request Sep 25, 2022
# Objective

I was about to submit a PR to add these two examples to `bevy-website` and re-discovered the inconsistency.

Although it's not a major issue on the website where only the filenames are shown, this would help to visually distinguish the two examples in the list  because the names are very prominent.

This also helps out when fuzzy-searching the codebase for these files.

## Solution

Rename `shapes` to `2d_shapes`. Now the filename matches the example name, and the naming structure matches the 3d example.

## Notes

@Nilirad proposed this in #4613 (comment) but it had slipped away from my brain at that time.
james7132 pushed a commit to james7132/bevy that referenced this pull request Oct 19, 2022
# Objective

I was about to submit a PR to add these two examples to `bevy-website` and re-discovered the inconsistency.

Although it's not a major issue on the website where only the filenames are shown, this would help to visually distinguish the two examples in the list  because the names are very prominent.

This also helps out when fuzzy-searching the codebase for these files.

## Solution

Rename `shapes` to `2d_shapes`. Now the filename matches the example name, and the naming structure matches the 3d example.

## Notes

@Nilirad proposed this in bevyengine#4613 (comment) but it had slipped away from my brain at that time.
james7132 pushed a commit to james7132/bevy that referenced this pull request Oct 28, 2022
# Objective

I was about to submit a PR to add these two examples to `bevy-website` and re-discovered the inconsistency.

Although it's not a major issue on the website where only the filenames are shown, this would help to visually distinguish the two examples in the list  because the names are very prominent.

This also helps out when fuzzy-searching the codebase for these files.

## Solution

Rename `shapes` to `2d_shapes`. Now the filename matches the example name, and the naming structure matches the 3d example.

## Notes

@Nilirad proposed this in bevyengine#4613 (comment) but it had slipped away from my brain at that time.
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
# Objective

I was about to submit a PR to add these two examples to `bevy-website` and re-discovered the inconsistency.

Although it's not a major issue on the website where only the filenames are shown, this would help to visually distinguish the two examples in the list  because the names are very prominent.

This also helps out when fuzzy-searching the codebase for these files.

## Solution

Rename `shapes` to `2d_shapes`. Now the filename matches the example name, and the naming structure matches the 3d example.

## Notes

@Nilirad proposed this in bevyengine#4613 (comment) but it had slipped away from my brain at that time.
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
# Objective

- As requested here: bevyengine#4520 (comment)
- Make it easier to spot issues with built-in shapes

## Solution

https://user-images.githubusercontent.com/200550/165624709-c40dfe7e-0e1e-4bd3-ae52-8ae66888c171.mp4

- Add an example showcasing the built-in 3d shapes with lighting/shadows
- Rotate objects in such a way that all faces are seen by the camera
- Add a UV debug texture

## Discussion

I'm not sure if this is what @alice-i-cecile had in mind, but I adapted the little "torus playground" from the issue linked above to include all built-in shapes.

This exact arrangement might not be particularly scalable if many more shapes are added. Maybe a slow camera pan, or cycling with the keyboard or on a timer, or a sidebar with buttons would work better. If one of the latter options is used, options for showing wireframes or computed flat normals might add some additional utility.

Ideally, I think we'd have a better way of visualizing normals.

Happy to rework this or close it if there's not a consensus around it being useful.
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-Examples An addition or correction to our examples 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