Skip to content

Merge shared functionality from render and compute passes #7838

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 5 commits into from
Jun 30, 2025

Conversation

Vecvec
Copy link
Collaborator

@Vecvec Vecvec commented Jun 23, 2025

Connections
Preparation for ray tracing pipelines (specifically the new pass that would be added).

Description
There were many duplicated functions, and some duplicated functionality between render and compute passes when actually recording the commands. This confused me, and I didn't realise there were two set_bind_group functions, and so some ray tracing validation was only in one. To implement ray tracing pipelines, a third pass would have to be introduced, which would cause a third duplication of this code.

Some things to note:

  • For some reason, compute passes used a tracker which took things from a usage scope, while render passes just used the usage scope directly. I couldn't find a reason this was necessary, but there might be a particular reason. re-added this.
  • Compute passes have an extra push constant field to reset push constants after indirect validation, there was a todo there, but I couldn't see why, and removed it as part of the refactor, but I can add it back if needed.

Testing
Tries to just keep all existing functionality.

Squash or Rebase?
Squash

Checklist

  • Run cargo fmt.
  • Run cargo clippy --tests.
  • Run cargo xtask test to run tests.
    • There were a few failures on my machine, but none seem to be introduced by this.
  • [N/a] If this contains user-facing changes, add a CHANGELOG.md entry. Internal changes only.

@Vecvec Vecvec requested a review from a team as a code owner June 23, 2025 01:18
@teoxoy teoxoy self-assigned this Jun 23, 2025
@teoxoy

This comment was marked as resolved.

@Vecvec

This comment was marked as resolved.

@Vecvec Vecvec force-pushed the merge-general-pass branch 5 times, most recently from a0982b3 to 14b0af2 Compare June 24, 2025 22:48
@andyleiserson
Copy link
Contributor

I am sorry about the conflicts from #7820. Some of the changes are just whitespace changes, it would probably help to look at that diff with whitespace changes ignored to get a sense of what's really going on. Let me know if I can help somehow.

I wonder if making things generic over errors like I did with validate_pass_timestamp_writes could help to avoid the extra GeneralPass layer in the errors?

@Vecvec
Copy link
Collaborator Author

Vecvec commented Jun 27, 2025

I wonder if making things generic over errors like I did with validate_pass_timestamp_writes could help to avoid the extra GeneralPass layer in the errors?

I hadn't thought of that, I'll give it a try.

@Vecvec Vecvec force-pushed the merge-general-pass branch 3 times, most recently from 7bdc1e0 to e827166 Compare June 27, 2025 04:26
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 overall - thanks for unifying these!

@Vecvec Vecvec force-pushed the merge-general-pass branch from e827166 to f55cc9a Compare June 27, 2025 19:34
@Vecvec Vecvec force-pushed the merge-general-pass branch from f55cc9a to b1056f2 Compare June 27, 2025 19:36
@Vecvec
Copy link
Collaborator Author

Vecvec commented Jun 27, 2025

Added unit structs for those errors. Definitely make stuff clearer.

@Vecvec Vecvec requested a review from teoxoy June 27, 2025 19:41
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.

Nice!

@teoxoy teoxoy merged commit 2721210 into gfx-rs:trunk Jun 30, 2025
40 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.

3 participants