Skip to content

Conversation

@andyleiserson
Copy link
Contributor

@andyleiserson andyleiserson commented Oct 17, 2025

Follow-up to #8220. @Vecvec pointed out that the behavior when mixing calls to the two APIs was changed by that PR. Since we don't have any tests for behavior when mixing the APIs, and don't know of use cases, it seems simplest to disallow it entirely.

Testing
Adds wgpu-validation tests.

Squash or Rebase? Squash

Checklist

  • Run cargo fmt.
  • Run taplo format.
  • Run cargo clippy --tests. If applicable, add:
    • --target wasm32-unknown-unknown
  • Run cargo xtask test to run tests.
  • Adds a CHANGELOG.md entry.

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.

Nice, thanks for the tests! This honestly makes things a lot less complicated with encoding.

@andyleiserson
Copy link
Contributor Author

andyleiserson commented Oct 20, 2025

I think that raw encoding may be broken since the encode-on-finish changes and fixed by this change, because of the assertion I added in CommandEncoderStatus::finish that the encoder has not yet been opened. That is the case for wgpu encoding, but not for raw encoding. Possibly we want to backport some form of this:

https://github.com/gfx-rs/wgpu/pull/8373/files#diff-1fe34f06c0c0bd67a41a9cb8aa26cae0a2a4b58028ce5e064abb6f8ad23b84cfR367-R371

CC @Vecvec

@Vecvec
Copy link
Contributor

Vecvec commented Oct 20, 2025

Possibly we want to backport some form of this

I think that that probably should be backported (especially since it is fairly early in the release cycle).

Should I also review this?

@andyleiserson
Copy link
Contributor Author

@Vecvec

Should I also review this?

If you have a minute to study whether the doc comment changes I made on mark_acceleration_structure_built makes sense, that would be useful. Other than that, I don't think you need to review the rest of it, IMO it pretty straightforwardly does what the title says. If you have test cases for raw encoding that I could use to check whether it is working on the v27 branch and on trunk, that would be useful.

@Vecvec
Copy link
Contributor

Vecvec commented Oct 20, 2025

It definitely is broken on v27, it hits assertion failed: !inner.encoder.is_open in my tests.

@cwfitzgerald cwfitzgerald self-assigned this Oct 22, 2025
@cwfitzgerald cwfitzgerald merged commit d575c02 into gfx-rs:trunk Oct 22, 2025
41 checks passed
@andyleiserson andyleiserson deleted the push-wwumxlspmtzv branch October 22, 2025 15:46
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