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

Add partial derivative builtins #2277

Merged
merged 3 commits into from
Mar 16, 2023
Merged

Add partial derivative builtins #2277

merged 3 commits into from
Mar 16, 2023

Conversation

evahop
Copy link
Contributor

@evahop evahop commented Mar 14, 2023

Adds the following from gfx-rs/wgpu#4402

  • dpdxCoarse
  • dpdxFine
  • dpdyCoarse
  • dpdyFine
  • fwidthCoarse
  • fwidthFine

Feel free to make any changes btw.

@evahop
Copy link
Contributor Author

evahop commented Mar 15, 2023

Clippy is freaking out for me locally, so I'm just force pushing now that I've appeased the CI

running cargo clean resolved most of my issues, however I still get:

error: lint `clippy::derive_hash_xor_eq` has been renamed to `clippy::derived_hash_with_manual_eq`
   --> src/back/msl/sampler.rs:159:9
    |
159 | #[allow(clippy::derive_hash_xor_eq)]
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use the new name: `clippy::derived_hash_with_manual_eq`
    |
    = note: `-D renamed-and-removed-lints` implied by `-D warnings`

error: could not compile `naga` (lib) due to previous error

Which even the CI suggested, however it then complained after I made the switch, and then didn't complain when I reverted it. 😵‍💫

Copy link
Member

@teoxoy teoxoy left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!
Overall it looks good!

One nit: Please try to to avoid using _ for match arms especially when that coincides with one remaining enum variant.

src/lib.rs Outdated Show resolved Hide resolved
Copy link
Member

@teoxoy teoxoy left a comment

Choose a reason for hiding this comment

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

misclicked

@evahop
Copy link
Contributor Author

evahop commented Mar 15, 2023

Thanks for the PR! Overall it looks good!

One nit: Please try to to avoid using _ for match arms especially when that coincides with one remaining enum variant.

Sounds good, does this also apply to resting the pattern if there's only one remaining field { x , .. } vs { x, y: _ }?

@evahop evahop requested a review from teoxoy March 15, 2023 22:27
@teoxoy
Copy link
Member

teoxoy commented Mar 16, 2023

Sounds good, does this also apply to resting the pattern if there's only one remaining field { x , .. } vs { x, y: _ }?

I think that's fine and depends on context; sometimes it might be worth explicitly ignoring members, sometimes we might not care at all and use the rest operator.

Copy link
Member

@teoxoy teoxoy left a comment

Choose a reason for hiding this comment

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

A rebase should take care of the linting issues; since we landed #2279.

src/back/dot/mod.rs Outdated Show resolved Hide resolved
src/back/hlsl/writer.rs Outdated Show resolved Hide resolved
src/back/spv/block.rs Outdated Show resolved Hide resolved
src/back/dot/mod.rs Outdated Show resolved Hide resolved
@gfx-rs gfx-rs deleted a comment from codecov-commenter Mar 16, 2023
src/back/dot/mod.rs Outdated Show resolved Hide resolved
Copy link
Member

@teoxoy teoxoy left a comment

Choose a reason for hiding this comment

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

Looks great - thank you for the improvements!

@teoxoy teoxoy merged commit 6db8da7 into gfx-rs:master Mar 16, 2023
kvark pushed a commit to kvark/naga that referenced this pull request Mar 18, 2023
* Add partial derivative builtins

* [dot] emit derivative control

* Fix fmt

---------

Co-authored-by: Teodor Tanasoaia <28601907+teoxoy@users.noreply.github.com>
@evahop evahop deleted the deriv branch September 28, 2023 14:35
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