Skip to content

Remove Shader weak_handles from bevy_pbr meshlets (with one exception). #19368

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

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

Conversation

andriyDev
Copy link
Contributor

Objective

Solution

  • Use the new load_shader_library macro for the shader libraries and embedded_asset/load_embedded_asset for the "shader binaries" in bevy_pbr meshlets.

The last piece that I haven't handled here is MESHLET_VISIBILITY_BUFFER_RESOLVE_SHADER_HANDLE. This is in an awkward situation that prevents us from using the standard solutions I've been using thus far. Essentially, a shader with this import path needs to be present, but we also need to be able to replace it if meshlets really are loaded. Embedded assets don't really play nicely with that, so we will need to figure out a solution there (or maybe wait for WESL to hopefully resolve this situation).

Testing

  • meshlet example still works.

P.S. I don't think this needs a migration guide. Technically users could be using the pub weak handles, but there's no actual good use for them, so omitting it seems fine. Alternatively, we could mix this in with the migration guide notes for #19137.

@andriyDev andriyDev requested a review from atlv24 May 25, 2025 23:52
@andriyDev andriyDev added A-Rendering Drawing game state to the screen A-Assets Load files from disk to use for things like images, models, and sounds D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels May 25, 2025
@atlv24
Copy link
Contributor

atlv24 commented May 26, 2025

I would prefer to just merge #19318 and avoid merge conflicts

@andriyDev
Copy link
Contributor Author

@atlv24 I get not wanting to deal with merge conflicts, but I'd prefer these two things be split. That way we can revert each independently if necessary.

But hey if that PR gets in first, I'm ok to close this one.

@atlv24
Copy link
Contributor

atlv24 commented May 26, 2025

meshlets is experimental, i dont think breakages warrant reverting there, especially this early in the cycle. if something breaks ill fix it, id really prefer not to rebase that work as its not all mine

@andriyDev andriyDev requested a review from JMS55 May 28, 2025 03:30
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 D-Straightforward Simple bug fixes and API improvements, docs, test and examples 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.

3 participants