Skip to content

Move incrementation of Device::last_acceleration_structure_build_command_index into queue submit #7462

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

Merged
merged 2 commits into from
Apr 10, 2025

Conversation

Vecvec
Copy link
Collaborator

@Vecvec Vecvec commented Apr 1, 2025

Connections
Fixes #5978 as well

Description
Previously, the acceleration structure build command index was incremented at the start of a build, but in certain cases (one is demonstrated by the test) this could cause odd issues. The first idea was to increment it at build submit, but that ran into a similar issue to #5978. Since the solution required the same things as queue submit I ended up fixing that too. Though I can remove that fix if it is wanted.

Testing
Adds a test for this

Squash or Rebase?
Squash

Checklist

  • Run cargo fmt.
  • Run cargo clippy --tests. If applicable, add:
  • Run cargo xtask test to run tests.
  • If this contains user-facing changes, add a CHANGELOG.md entry.

@Vecvec Vecvec requested a review from a team as a code owner April 1, 2025 18:32
@cwfitzgerald cwfitzgerald self-assigned this Apr 2, 2025
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.

I think this code looks fine - would it be possible to rebase into two commits, one for the submission index change, then one for the rest of the RT stuff. Due to how finicky this stuff is, the submission change being isolated would be good for bisection.

Conditionally approving.

@Vecvec
Copy link
Collaborator Author

Vecvec commented Apr 9, 2025

I'm not entirely sure how to do that, but I'll try.

@Vecvec Vecvec force-pushed the fix-build-load branch 2 times, most recently from 5696a3e to aa3c69d Compare April 9, 2025 20:56
@Vecvec
Copy link
Collaborator Author

Vecvec commented Apr 9, 2025

Does this work better?

Edit: Nvm still need to fix CI.

@Vecvec Vecvec force-pushed the fix-build-load branch 3 times, most recently from aaa29d6 to a4ca035 Compare April 10, 2025 00:36
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.

Sorry one last issue.

@cwfitzgerald
Copy link
Member

Thanks!

@cwfitzgerald cwfitzgerald merged commit 8010203 into gfx-rs:trunk Apr 10, 2025
37 checks passed
@Vecvec Vecvec deleted the fix-build-load branch April 10, 2025 03:37
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.

wgpu-core might not submit commands to hal queue with strictly increasing submission indices
2 participants