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

Reintroduce computepass->encoder lifetime constraint and make it opt-out via wgpu::ComputePass::forget_lifetime #5768

Merged
merged 8 commits into from
Jun 3, 2024

Conversation

Wumpf
Copy link
Member

@Wumpf Wumpf commented Jun 2, 2024

Connections

Description
As suggested on Matrix chat by @kpreid.

In #5620, wgpu::ComputePass lost its explicit lifetime constraint to wgpu::CommandEncoder. This constraint prevented use of the command encoder while a pass is recorded. This is still an error, but now a runtime error. While removing this lifetime is very useful especially for library authors, it also prevented trivial incorrect api usage, i.e. that PR was great but also created a massive footgun ;-).

Here, we remedy this issue by re-introducing the exact same lifetime constraint but making it opt-out with a zero-overhead forget_lifetime method.

Testing
Existing tests have been adjusted - compilation failed as expected without these adjustments!

Checklist

  • Run cargo fmt.
  • 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.

@Wumpf Wumpf requested a review from a team as a code owner June 2, 2024 10:54
wgpu/src/lib.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@kpreid kpreid left a comment

Choose a reason for hiding this comment

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

You pinged me, so you get my review 😁

The most important comment in this review bundle is that I think the name make_static() needs work.

wgpu/src/lib.rs Outdated Show resolved Hide resolved
wgpu/src/lib.rs Outdated Show resolved Hide resolved
wgpu/src/lib.rs Show resolved Hide resolved
wgpu/src/lib.rs Outdated Show resolved Hide resolved
wgpu/src/lib.rs Outdated Show resolved Hide resolved
wgpu/src/lib.rs Outdated Show resolved Hide resolved
wgpu/src/lib.rs Outdated Show resolved Hide resolved
wgpu/src/lib.rs Outdated Show resolved Hide resolved
wgpu/src/lib.rs Outdated Show resolved Hide resolved
@Wumpf Wumpf requested a review from kpreid June 2, 2024 15:09
CHANGELOG.md Outdated Show resolved Hide resolved
wgpu/src/lib.rs Outdated Show resolved Hide resolved
wgpu/src/lib.rs Outdated Show resolved Hide resolved
wgpu/src/lib.rs Outdated Show resolved Hide resolved
wgpu/src/lib.rs Outdated Show resolved Hide resolved
wgpu/src/lib.rs Outdated Show resolved Hide resolved
wgpu/src/lib.rs Outdated Show resolved Hide resolved
@Wumpf Wumpf changed the title Reintroduce computepass->encoder lifetime constraint and make it opt-out via wgpu::ComputePass::make_static Reintroduce computepass->encoder lifetime constraint and make it opt-out via wgpu::ComputePass::forget_lifetime Jun 2, 2024
@Wumpf
Copy link
Member Author

Wumpf commented Jun 2, 2024

thanks again, I'm much happier with this now!

@Wumpf Wumpf requested a review from kpreid June 2, 2024 21:11
@ErichDonGubler ErichDonGubler self-assigned this Jun 3, 2024
@ErichDonGubler ErichDonGubler added the kind: refactor Making existing function faster or nicer label Jun 3, 2024
@Wumpf Wumpf merged commit aa2821b into gfx-rs:trunk Jun 3, 2024
25 checks passed
@Wumpf Wumpf deleted the make-static-computepass branch June 3, 2024 18:04
@ErichDonGubler
Copy link
Member

ErichDonGubler commented Jun 4, 2024

AFAICT, controls to Approve this PR are inaccessible. But ✅ let it be known that I would do so after reading through this today. Thanks for the help, @kpreid!

@Wumpf Wumpf mentioned this pull request Jun 26, 2024
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: refactor Making existing function faster or nicer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants