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

Relax lifetimes on TextRenderer::render() #116

Merged
merged 2 commits into from
Oct 1, 2024
Merged

Conversation

Selicre
Copy link
Contributor

@Selicre Selicre commented Sep 30, 2024

The method's lifetimes are far too restrictive, which makes the method really annoying to use. I'm not sure if you have any plans to make use of these restrictions in the future, but I don't see why you would need to.

@grovesNL
Copy link
Owner

Could you explain a use case that can't use the existing lifetimes? We're basically trying to follow the pattern in https://github.com/gfx-rs/wgpu/wiki/Encapsulating-Graphics-Work#render

@ScanMountGoat
Copy link

Could you explain a use case that can't use the existing lifetimes?

I'm having issues trying to get code to compile with this crate when defining callbacks with wgpu 22.1.0, glyphon 0.6.0, and egui 0.29.0 due to the lifetime restrictions. The WGPU lifetime changes are explained in this PR: emilk/egui#5149

@Selicre
Copy link
Contributor Author

Selicre commented Sep 30, 2024

This primarily matters when you erase the lifetime on the pass using RenderPass::erase_lifetime(). It will effectively require all your lifetimes to be 'static, which is usually not desirable.

@grovesNL
Copy link
Owner

Ah ok, that makes sense - if you're erasing the render pass lifetime to 'static then this will break for sure.

erase_lifetime wasn't supported in earlier wgpu versions so it wasn't a problem until now. Let's go ahead with the elided lifetimes in that case.

Copy link
Owner

@grovesNL grovesNL left a comment

Choose a reason for hiding this comment

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

Thanks!

@grovesNL grovesNL merged commit 0115197 into grovesNL:main Oct 1, 2024
1 check 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