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 user supplied mesh tag #17648

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

tychedelia
Copy link
Contributor

@tychedelia tychedelia commented Feb 2, 2025

Objective

Because of mesh preprocessing, users cannot rely on @builtin(instance_index) in order to reference external data, as the instance index is not stable, either from frame to frame or relative to the total spawn order of mesh instances.

Solution

Add a user supplied mesh index that can be used for referencing external data when drawing instanced meshes.

Closes #13373

Testing

Benchmarked many_cubes showing no difference in total frame time.

Showcase

Screen.Recording.2025-02-02.at.12.29.35.PM.mov

Because of mesh preprocessing, users cannot rely on `@builtin(instance_index)`
in order to reference external data, as the instance index is not stable,
either from frame to frame or relative to the total spawn order of mesh instances.

Add a user supplied mesh index that can be used for referencing external data
when drawing instanced meshes.

Fixes bevyengine#13373
@tychedelia tychedelia added C-Feature A new feature, making something new possible A-Rendering Drawing game state to the screen S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Feb 2, 2025
@tychedelia tychedelia added this to the 0.16 milestone Feb 2, 2025
Copy link
Contributor

@pcwalton pcwalton left a comment

Choose a reason for hiding this comment

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

In the decal code I have a similar feature, but I called it the "tag". I think that might be a less confusing name than "instance index", because with your code as it currently stands "instance index" has two separate meanings.

Copy link
Contributor

github-actions bot commented Feb 2, 2025

You added a new example but didn't add metadata for it. Please update the root Cargo.toml file.

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.

I have no idea if the slightly higher memory usage is something we want to care about or not, but other than that this is a really useful PR and the implementation is nice and simple.

@IceSentry
Copy link
Contributor

IceSentry commented Feb 2, 2025

Yes, I agree that the name is a bit confusing though. I understand why it's named like that and it is factually correct, but potentially confusing.

@tychedelia
Copy link
Contributor Author

Okay, I think "tag" is a nice alternative for sure especially if it's already used in the engine, I'll switch to that instead 👍 .

@JMS55
Copy link
Contributor

JMS55 commented Feb 2, 2025

I somewhat wish we could instead always have a tag corresponding to an Entity, and have the instance_index builtin be used to index into a list of tags. So you do draw instance_index within a phase -> tags[instance_index] -> transforms/other_data[tag].

I think we've discussed this on discord in the past, don't remember the conclusion we came to.

@tychedelia tychedelia changed the title Add user supplied mesh instance index Add user supplied mesh tag Feb 2, 2025
@tychedelia tychedelia requested a review from pcwalton February 2, 2025 22:12
@tychedelia
Copy link
Contributor Author

tychedelia commented Feb 2, 2025

I somewhat wish we could instead always have a tag corresponding to an Entity, and have the instance_index builtin be used to index into a list of tags. So you do draw instance_index within a phase -> tags[instance_index] -> transforms/other_data[tag].

We just discussed this on discord. This definitely makes sense as a design and could help unify some of these patterns. @IceSentry brought up shader mesh picking as an example of something that could be useful for the engine to ship. I'd be happy if someone implemented that but it will require deeper incisions into things and make my primary use case more complicated. This implementation is still very general and can be used to implement other similar things (e.g. write the entity index into the tag) as well. I do think storing the entity on the gpu may be somewhat inevitable but I'd like to unblock this first, especially to see what other user needs arise.

@@ -1,51 +1,97 @@
//! Shows that multiple instances of a cube are automatically instanced in one draw call
//! Try running this example in a graphics profiler and all the cubes should be only a single draw call.
Copy link
Contributor

Choose a reason for hiding this comment

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

I just did a quick sanity check and it is indeed still a single draw call 🎉

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-Feature A new feature, making something new possible S-Needs-Review Needs reviewer attention (from anyone!) to move forward
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

User provided mesh binding / per-instance mesh data
4 participants