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

Implement Clone for ShaderModuleDescriptor #2902

Closed

Conversation

daxpedda
Copy link
Contributor

@daxpedda daxpedda commented Jul 21, 2022

Checklist

  • Run cargo clippy.
  • Run RUSTFLAGS=--cfg=web_sys_unstable_apis cargo clippy --target wasm32-unknown-unknown if applicable.
  • Add change to CHANGELOG.md. See simple instructions inside file.

Connections
Depends on gfx-rs/naga#2013.

Description
I am playing around with pre-compiled shaders and encountered an issue with storing them in statics, ShaderSource::Naga needs to own it, so cloning solves the problem.

Testing
None.

@daxpedda daxpedda force-pushed the shader-module-descriptor-clone branch from 21c7f40 to 688aae8 Compare July 21, 2022 11:20
@daxpedda daxpedda force-pushed the shader-module-descriptor-clone branch from 688aae8 to 964e9d6 Compare August 7, 2022 23:14
@daxpedda
Copy link
Contributor Author

Now that gfx-rs/naga#2013 was merged, I updated the relevant parts to depend on it.

To explain this PR a bit further, personally it doesn't make sense to me without #2903. This PR can further refine the process by not having the user build ShaderModuleDescriptor from scratch every time, a very small quality-of-life improvement.

With this PR:

const SHADER: &'static ShaderModuleDescriptor = ...;

// The module itself is not cloned here, because it uses `Cow::Borrowed(..)`.
device.create_shader_module(SHADER.clone());

Without this PR:

const MODULE: &'static Module = ...;

// Now you have to write it out, even though it could be part of the const.
device.create_shader_module(ShaderModuleDescriptor {
    label: None,
    source: ShaderSource::Naga(Cow::Borrowed(MODULE)),
});

I must say after writing out this example and already locally using #2903 successfully, I don't think the quality-of-life improvement this PR offers is good. Calling clone requires the user to actually understand that nothing is cloned in a case like this, which to me adds unnecessary mental load when reading through code.

Maybe somebody else can see the value in this, otherwise feel free to close.

@daxpedda daxpedda force-pushed the shader-module-descriptor-clone branch from 964e9d6 to e16b5ce Compare August 25, 2022 08:04
@daxpedda daxpedda marked this pull request as ready for review August 25, 2022 08:04
@cwfitzgerald
Copy link
Member

Closing in favor of #2903

@daxpedda
Copy link
Contributor Author

Just for clarification, as I said, I don't have a stake in this anymore: #2903 doesn't replace this. This PR (#2902) adds more quality-of-life to #2903.

@daxpedda
Copy link
Contributor Author

daxpedda commented Oct 8, 2022

@cwfitzgerald just wanted to make sure you take a second look at this after #2890 was merged. I can rebase this PR if it is still desired.

Copy link
Member

@cwfitzgerald cwfitzgerald left a comment

Choose a reason for hiding this comment

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

Yes please rebase, then we're g2g

@daxpedda
Copy link
Contributor Author

daxpedda commented Oct 8, 2022

@cwfitzgerald done, you probably have to reopen for CI to run.

@cwfitzgerald
Copy link
Member

cwfitzgerald commented Oct 8, 2022

Did you push? Force pushes should show up in the event log (and trigger CI)

@daxpedda
Copy link
Contributor Author

daxpedda commented Oct 8, 2022

Yeah I pushed, I think nothing shows up because the PR is closed. I can open another one?

@cwfitzgerald
Copy link
Member

Huh, yeah go ahead, I can't re-open it either

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.

2 participants