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

Naga 0.20 #87

Open
wants to merge 15 commits into
base: master
Choose a base branch
from
Open

Naga 0.20 #87

wants to merge 15 commits into from

Conversation

Elabajaba
Copy link

@Elabajaba Elabajaba commented Apr 29, 2024

Pipeline override constants will be fully implemented in a followup PR.

They're theoretically working, but they need tests and also need a function to map the unmangled names to the mangled names.

Otherwise this appears to work. I've tested it with bevy and everything seemed to work.

@Elabajaba
Copy link
Author

Discussed this with robtfm on discord, and there seems to be 2 ways forwards for supporting pipeline overridable constants.

  1. Mangle them in the shaders, and provide a function that you'd have to use to get the same mangling for them in the rust side code when you pass them into your pipeline descriptors. (seemed to be robtfm's preferred way). This should still let you refer to them by their ids without having to use the mangle function.
  2. Don't mangle them, require that they're globally unique (which is what naga_oil's current shader defs require), and add a bunch of checks in the compose module to opt out of mangling them.

…ga requires them to be a single Arena, and merging arenas sucks.
@Elabajaba Elabajaba marked this pull request as ready for review May 17, 2024 20:35
Copy link
Collaborator

@robtfm robtfm left a comment

Choose a reason for hiding this comment

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

just a couple of minor nits on derive.

i also wanted to keep the prune support so i've done that. i made a pr to your branch with the comment changes and the prune code

src/derive.rs Outdated Show resolved Hide resolved
src/derive.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@robtfm robtfm left a comment

Choose a reason for hiding this comment

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

also note - the function to map pipeline const names is already there - Composer::decorated_name does the job.

thanks very much for taking this on.

@Elabajaba
Copy link
Author

also note - the function to map pipeline const names is already there - Composer::decorated_name does the job.

thanks very much for taking this on.

🤦

Copy link
Contributor

@JMS55 JMS55 left a comment

Choose a reason for hiding this comment

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

Haven't actually tested subgroups/pipeline overrides, but approving it conditional on it working. I'll have to hook this up to bevy and try it out when I'm next free.

Copy link

@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 haven't seen anything obviously wrong in the code and I tested it with almost all 2d and 3d examples with no issues. So LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants