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

Add sprite and mesh alteration examples #15298

Merged

Conversation

richchurcher
Copy link
Contributor

Objective

Add examples for manipulating sprites and meshes by either mutating the handle or direct manipulation of the asset, as described in #15056.

Closes #3130.

(The previous PR suffered a Git-tastrophe, and was unceremoniously closed, sry! 😅 )

Copy link
Contributor

The generated examples/README.md is out of sync with the example metadata in Cargo.toml or the example readme template. Please run cargo run -p build-templated-pages -- update examples to update it, and commit the file change.

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.

Perfect: this demonstrates the point really nicely.

@alice-i-cecile alice-i-cecile added A-Rendering Drawing game state to the screen C-Examples An addition or correction to our examples A-Assets Load files from disk to use for things like images, models, and sounds S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Sep 19, 2024
Copy link
Member

@janhohenheim janhohenheim left a comment

Choose a reason for hiding this comment

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

Good idea! The alter_mesh example could mayyyybe be turned into procedural_mesh, as that is what most people using this API will probably end up doing. Not sure, your choice.
The rest of the comments are just about how to make the examples a bit easier to understand for outsiders. Comments are mostly valid for both examples.
Good job with the examples, they're pretty nicely written!

examples/asset/alter_mesh.rs Outdated Show resolved Hide resolved
examples/asset/alter_mesh.rs Outdated Show resolved Hide resolved
examples/asset/alter_mesh.rs Outdated Show resolved Hide resolved
examples/asset/alter_mesh.rs Outdated Show resolved Hide resolved
examples/asset/alter_mesh.rs Outdated Show resolved Hide resolved
@JMS55
Copy link
Contributor

JMS55 commented Sep 20, 2024

I'm surprised not to see any usage of RenderAssetUsages here.

@richchurcher
Copy link
Contributor Author

I'm surprised not to see any usage of RenderAssetUsages here.

I'm sorry, you might need to ELI5 here... how should I best use this?

@richchurcher
Copy link
Contributor Author

could mayyyybe be turned into procedural_mesh, as that is what most people using this API will probably end up doing.

Couldn't make a decision either way, so left it alone.

@JMS55
Copy link
Contributor

JMS55 commented Sep 20, 2024

I'm sorry, you might need to ELI5 here... how should I best use this?

Read the docs https://dev-docs.bevyengine.org/bevy/render/render_asset/struct.RenderAssetUsages.html, and then let me know if you have any questions.

I just realized the default is actually RenderAssetUsages::all() so that's why this example is fine, but I'd prefer if we could explicitly set that to all() and call out why it's necessary, as I very frequently see people make the mistake of using RENDER_WORLD only and then being confused why their asset does not exist.

// assets available in `Res<Assets<Mesh>>`. `RENDER_WORLD` alone will cause the asset to be
// unloaded from the asset server after it's been sent to the GPU. Unless you have a clear
// reason not to do so, it's best to use `RenderAssetUsages::all().
|settings: &mut GltfLoaderSettings| settings.load_meshes = RenderAssetUsages::all(),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@JMS55 Is this the kind of thing you had in mind? New territory for me, so feel free to provide more direction!

Copy link
Member

@janhohenheim janhohenheim Sep 20, 2024

Choose a reason for hiding this comment

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

@JMS55 will know better than me, but IME it is the other way around: usually you won't be mutating meshes at runtime, so it's best to set it to RENDER_WORLD only unless you have a good reason to set it to ALL. Although IIRC for footgun reasons, we left the default at ALL.

Copy link
Contributor Author

@richchurcher richchurcher Sep 21, 2024

Choose a reason for hiding this comment

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

So JMS55 wrote:

but I'd prefer if we could explicitly set that to all() and call out why it's necessary, as I very frequently see people make the mistake of using RENDER_WORLD only and then being confused why their asset does not exist.

Perhaps if I make a point of only setting this to all for the asset whose mesh we're going to directly modify, and re-word the comment to say the default is "usually safe" unless you have more specific needs? I'll push that so you can have a read.

Copy link
Member

@janhohenheim janhohenheim Sep 21, 2024

Choose a reason for hiding this comment

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

The default is not usually safe, it is always safe, as the default is ALL. It's just twice as expensive than it needs to be for 99% of use cases, as nearly no one will be editing or inspecting meshes at runtime. Many people choose to use RENDER_WORLD instead of ALL for meshes, which is safe until you need to access the mesh on the CPU, which we do in this example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm with you now. OK, so maybe the second one we explicitly set it to RENDER_WORLD and add a further comment there. Maybe also modify the sprite example.

Copy link
Member

@janhohenheim janhohenheim left a comment

Choose a reason for hiding this comment

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

Looks great now, thanks :) The example are very readable, good job!

@janhohenheim janhohenheim added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it labels Sep 20, 2024
@janhohenheim
Copy link
Member

Leaving it as waiting for author because of the render world thing

@richchurcher
Copy link
Contributor Author

Thanks for being patient, I think I've arrived at a good place with the comments now!

@alice-i-cecile alice-i-cecile added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Sep 21, 2024
@alice-i-cecile
Copy link
Member

I'm going to merge this once CI is green :)

@richchurcher
Copy link
Contributor Author

Argh internal imports again! One moment, caller.

@richchurcher
Copy link
Contributor Author

@alice-i-cecile sorted

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Sep 22, 2024
Merged via the queue into bevyengine:main with commit e3b6b12 Sep 22, 2024
30 checks passed
@janhohenheim
Copy link
Member

Thanks for being patient

If this took a moment for you, that's a big indicator that our existing docs could be better in this regard. Happy to have an example showing and explaining the concept now :)
Fantastic work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Assets Load files from disk to use for things like images, models, and sounds 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.

Examples to demonstrate changing a sprite and texture
4 participants