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

remove lifetimes on renderpass #4768

Closed
wants to merge 1 commit into from

Conversation

swiftcoder
Copy link
Contributor

Connections
The recent lifetime changes to RenderPass make it difficult to use. #1453

Description
Remove the lifetime parameters from RenderPass methods, as the lifetimes of handles should now be extended correctly behind the scenes

Testing
Ran a variety of the examples successfully

Attempted to run the test suite, but it hard reboots my Mac (MacBook pro 16" intel i9, with Radeon Pro 5500M)

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.

@swiftcoder swiftcoder requested a review from a team as a code owner November 24, 2023 13:32
@cwfitzgerald
Copy link
Member

cwfitzgerald commented Nov 24, 2023

Thanks for the PR!

This unfortunately doesn't actually fix the problem. Arcanization didn't remove the need for lifetimes, it allowed us to potentially remove them. Unfortunately it's not a trivial transform to get rid of the need for lifetimes as you need some level of validation before you can clone the arc. There's also some ramifications for Firefox, so we're going to discuss this in the next maintainers meeting.

The test suite rebooting your computer is a smidge concerning 👀

@MendyBerger
Copy link

@cwfitzgerald any decisions coming from that meeting? Would really like to see those lifetimes removed.

@cwfitzgerald
Copy link
Member

Basically we can do this without needing those changes. The real changes will be making sure performance doesn't fall.

@cwfitzgerald
Copy link
Member

If you'd be interested in working on this, I can go into more detail tomorrow

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