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] Explicitly use global const ctx in offset #6945

Merged
merged 2 commits into from
Jan 20, 2025

Conversation

sagudev
Copy link
Contributor

@sagudev sagudev commented Jan 18, 2025

Connections
Preparation for #6935

Description
Most of wgsl-in assumes constants are only global. In order to fix this in #6935 this PR makes TetureSampler offset explicitly use global const ctx and removes as_const on expr ctx.

Testing
This is tested by existing tests

Checklist

  • Run cargo fmt.
  • Run taplo format.
  • Run cargo clippy. If applicable, add:
    • --target wasm32-unknown-unknown
    • --target wasm32-unknown-emscripten
  • Run cargo xtask test to run tests.
  • Add change to CHANGELOG.md. See simple instructions inside file.

Signed-off-by: sagudev <16504129+sagudev@users.noreply.github.com>
Signed-off-by: sagudev <16504129+sagudev@users.noreply.github.com>
@sagudev sagudev requested a review from a team as a code owner January 18, 2025 10:07
Comment on lines +375 to 376
#[allow(dead_code)]
fn as_const(&mut self) -> ExpressionContext<'source, '_, '_> {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It might make sense to remove this and add it back later.

Copy link
Member

Choose a reason for hiding this comment

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

I agree; the PR would become a one line change then.

I think it would be fine to fold it into #6935. Or would you still like to land this separately?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it would make sense to land it as it stands now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or maybe not given how clippy/rustdoc hates me, but I would still like to land this separately. Reverted to initial version.

Copy link
Member

Choose a reason for hiding this comment

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

That's fair.

@sagudev sagudev changed the title [naga] Use global const ctx in offset [naga] Explicitly use global const ctx in offset Jan 18, 2025
@teoxoy teoxoy self-assigned this Jan 20, 2025
@sagudev sagudev force-pushed the try-const branch 2 times, most recently from afb5706 to d0ce4e1 Compare January 20, 2025 17:47
@teoxoy teoxoy merged commit 6e7d3c4 into gfx-rs:trunk Jan 20, 2025
58 checks passed
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