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 ddx, ddy, fwidth #256

Merged
merged 1 commit into from
Nov 19, 2020
Merged

Implement ddx, ddy, fwidth #256

merged 1 commit into from
Nov 19, 2020

Conversation

khyperia
Copy link
Contributor

Does things relevant to #197 (uh, avoiding the github magic word that closes the issue, since this doesn't deal with all of it)

Now that asm! is in, we don't even have to touch the compiler to implement this! wow!

bikesheddables:

  • Function names? I copied the names from the spir-v spec.
  • What to do on CPU? Currently panics, because the alternative of cfg'ing them out makes the IDE experience horrible (since IDE uses host target)

Things that are technically bikesheddables but I feel strongly about:

  • Where do we put this? I think we should keep it in spirv-std in a specific extension trait, because it actually is extremely spirv-specific, and that's what spirv-std is for.

@khyperia khyperia requested a review from repi November 18, 2020 13:53
Comment on lines +156 to +161
// "must be a scalar or vector of floating-point type. The component width must be 32 bits."
deriv_impl!(f32);
// TODO: Fix rustc to support these
// deriv_impl!(glam::Vec2);
// deriv_impl!(glam::Vec3);
// deriv_impl!(glam::Vec4);
Copy link
Contributor

Choose a reason for hiding this comment

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

Just confirmed that these lines (uncommented) stop erroring with rust-lang/rust#79171.

Copy link
Contributor

@repi repi left a comment

Choose a reason for hiding this comment

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

Think it is a great fit to have this in spirv-std, or at least it is the best place for it now.

Wrt to function names and other such bikeshedding. Think we can just start with this and having the functionality, and then once we've used it a bit evaluate it further and see if we want to rename stuffs.

In general on the CPU I'm in favor of getting compile errors for things that are not implemented, but the general Rust ecosystem philosophy is indeed to have stuff compile and get panics instead. I would probably still lean towards simply not having this available as I really don't want my shaders when running on CPU to be panicking, that type of porting I would want compile errors for and we are in a bit more controlled ecosystem.

But definitely are tradeoffs with IDE/RA also. Could potentially be a Cargo feature flag for the crate also of which behavior to have, probably by default not having the functions available on CPU, but opting in to having them defined but getting panics.

But we'll have to see what works best

@mergify mergify bot merged commit fcef9ba into main Nov 19, 2020
@mergify mergify bot deleted the derivatives branch November 19, 2020 12:42
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.

3 participants