More shader/runner combos #404
Conversation
nnethercote
requested review from
eddyb,
LegNeato,
Firestar99 and
schell
as code owners
examples/runners/ash/src/main.rs
Outdated
| print_backtrace: true, | ||
| }) | ||
| // HACK(eddyb) needed because of `debugPrintf` instrumentation limitations | ||
| // (see https://github.com/KhronosGroup/SPIRV-Tools/issues/4892). |
There was a problem hiding this comment.
KhronosGroup/SPIRV-Tools#4892 appears to be closed, this might be fixed in our version of spirv-tools now?
There was a problem hiding this comment.
Oops, forgot about the ash runner in:
(that PR's only change to any code is pretty much just removing this kind of special-case, but only from the wgpu runner)
There was a problem hiding this comment.
Fixing this in the ash runner is more complicated than in the WGPU runner. Elsewhere the ash runner uses unwrap_multi. If this .multimodule(true) is removed then that unwrap_multi must be changed to unwrap_single, which has a bunch of knock-on effects. It might be possible, but I would like to look into it as a follow-up because it's beyond the scope of this PR.
There was a problem hiding this comment.
I can do the ash part if you want to (since I'm familiar with vulkan)
There was a problem hiding this comment.
It's fine to keep using multimodule for now, can just leave an extra comment (or replace the existing one), clarifying the combination of:
- it's no longer needed
- refactoring it from multi back to single is non-trivial and hasn't been done yet
There was a problem hiding this comment.
I looked into the multimodule removal, but I couldn't get it to work. The sticking point was here -- the vertex shader and fragment shader need to be separate (as far as I can tell), and I can't see how to do that without multimodule.
While I was trying to eliminate the multimodule I found a bunch of other simplifications that can be made to the ash runner. (It has unnecessary support for multiple pipelines.) Once this PR merges I will file a PR for those simplifications, so @Firestar99 I suggest waiting until after that before looking at multimodule. Thanks!
@eddyb: I will update the comment.
| } | ||
|
|
||
| // The form with underscores, e.g. `sky_shader`. | ||
| fn crate_ident(&self) -> &'static str { |
There was a problem hiding this comment.
This can probably just be self.crate_name().replace("-", "_") (it's only used w/ format! AFAICT, so there's already an inherent string allocation cost anyway).
There was a problem hiding this comment.
Eh, but it has to return a String, which would make it different to crate_name, and I have a follow-up that will eliminate this anyway. So I'll leave it as is.
examples/runners/ash/src/main.rs
Outdated
| print_backtrace: true, | ||
| }) | ||
| // HACK(eddyb) needed because of `debugPrintf` instrumentation limitations | ||
| // (see https://github.com/KhronosGroup/SPIRV-Tools/issues/4892). |
There was a problem hiding this comment.
It's fine to keep using multimodule for now, can just leave an extra comment (or replace the existing one), clarifying the combination of:
- it's no longer needed
- refactoring it from multi back to single is non-trivial and hasn't been done yet
The background should be green, not blue, to match the wgpu runner.
mouse-shader doesn't work fully. But it's an example, so partial function is still reasonable.
nnethercote
force-pushed
the
more-shader-runner-combos
branch
from
5ddf33d to
3c08300
Compare
|
Ok, I have added a new commit that updates the comment on the |
|
@eddyb has approved the changes. Any other review comments? |
|
This PR now has approval from @eddyb and @LegNeato. @Firestar99, @schell: any comments? Or if someone wants to merge it, that would be great. I don't have permission to do that. |
Just expanding the examples a little.