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

Renderpass take resource ownership #5884

Merged
merged 15 commits into from
Jul 1, 2024

Conversation

Wumpf
Copy link
Member

@Wumpf Wumpf commented Jun 26, 2024

Connections

Description
Just like ComputePass before, we now take ownership of all ingoing resources to a RenderPass right away. We also take ownership of the parent encoder, making it possible to convert RenderPass<'a>->RenderPass<'static> (as opt-in)

Testing
New ownership tests added (very similar to what we added for compute pass previously)

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 force-pushed the renderpass-take-resource-ownership branch from a7bec42 to c097717 Compare June 29, 2024 11:55
@Wumpf Wumpf force-pushed the renderpass-take-resource-ownership branch 2 times, most recently from d10100d to bdc6328 Compare June 30, 2024 12:31
@Wumpf Wumpf force-pushed the renderpass-take-resource-ownership branch from 6347d28 to ebbbdf2 Compare June 30, 2024 12:57
@Wumpf Wumpf force-pushed the renderpass-take-resource-ownership branch from ebbbdf2 to a6bcbf1 Compare June 30, 2024 13:05
@Wumpf Wumpf force-pushed the renderpass-take-resource-ownership branch from a6bcbf1 to a463cdd Compare June 30, 2024 13:07
@Wumpf Wumpf marked this pull request as ready for review June 30, 2024 13:14
@Wumpf Wumpf requested a review from a team as a code owner June 30, 2024 13:14
@Wumpf Wumpf merged commit 0a76c0f into gfx-rs:trunk Jul 1, 2024
25 checks passed
@Wumpf Wumpf deleted the renderpass-take-resource-ownership branch July 1, 2024 16:36
@John-Nagle
Copy link

It turns out that this change badly broke Rend3. See

https://users.rust-lang.org/t/unexpected-borrowed-data-escapes-outside-of-closure-for-generic/119501/2

(Rend3 was abandoned by its author around the time this went in. But I'm stuck using it, and now I have to fix it.)

@grovesNL
Copy link
Collaborator

grovesNL commented Oct 11, 2024

@John-Nagle This PR loosens the lifetime requirements on the wgpu side. The change to 'static lifetime in egui makes you require forget_lifetime otherwise everything would work the same as usual.

You'll need to call forget_lifetime before you share renderpasses with egui. See emilk/egui#5149 for background and examples.

@John-Nagle
Copy link

I get that, mostly. The worst trouble is near here:

https://github.com/BVE-Reborn/rend3/blob/trunk/rend3/src/graph/graph.rs#L403

This code is trying to manage the lifetime of the render pass and encoder from unsafe code.
I'm still trying to figure this out. See the "SAFETY" comment:

// SAFETY: There are two things which borrow this encoder: the render pass and the node's
// encoder reference. Both of these have died by this point.

That's not an assurance that this is safe code. It's a warning that there are lifetime requirements imposed on the caller. Ones which are changing now. The code in that function does considerable unsafe manipulation of encoder and render pass lifetimes. So changes in this area are likely to cause undefined behavior.

I really don't want to touch that code if I can possibly avoid doing so. Is that possible?

@Wumpf
Copy link
Member Author

Wumpf commented Oct 13, 2024

@John-Nagle lifetime requirements only got relaxed and more things are now handled as a runtime error rather than a compile time error which is what allows forget_lifetime to be a safe operation. My expectation is that everything still works fine except that there's some lifetime annotations that can just go away - any lifetime annotation that ties a resource (e.g. pipeline) to the lifetime of a render pass should no longer be needed (and in some cases may cause compile errors).
From the little I know about rend3 via its author the unsafe code in this area is exactly because previously the ownership management was only via lifetime annotation & there was no forget_lifetime. But keeping the old unsafe code should still work fine afaik

@John-Nagle
Copy link

The unsafe code in this area is exactly because previously the ownership management was only via lifetime annotation & there was no forget_lifetime.

I think that's about right. It looks to me like the old WGPU lifetime system forced complex unsafe ownership management. I'm trying to figure that out now. I think ordinary Rc and RefCell can do the job now. Thanks.

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.

Lifetimes on RenderPass make it difficult to use.
4 participants