8000 Relax lifetimes on TextRenderer::render() by Selicre · Pull Request #116 · grovesNL/glyphon · GitHub
[go: up one dir, main page]
Skip to content

Relax lifetimes on TextRenderer::render() - #116

Merged
grovesNL merged 2 commits into
grovesNL:mainfrom
Selicre:render-lt
Oct 1, 2024
Merged

Relax lifetimes on TextRenderer::render()#116
grovesNL merged 2 commits into
grovesNL:mainfrom
Selicre:render-lt

Conversation

@Selicre
@Selicre Selicre commented Sep 30, 2024
Copy link
Copy Markdown
Contributor

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
grovesNL commented Sep 30, 2024
Copy link
Copy Markdown
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
ScanMountGoat commented Sep 30, 2024
Copy link
Copy Markdown

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
Selicre commented Sep 30, 2024
Copy link
Copy Markdown
Contributor Author

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
grovesNL commented Sep 30, 2024
Copy link
Copy Markdown
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.

grovesNL
grovesNL approved these changes Oct 1, 2024
View reviewed changes
@grovesNL grovesNL left a comment
Copy link
Copy Markdown
Owner

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@grovesNL grovesNL grovesNL approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Selicre @grovesNL @ScanMountGoat
0