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

Update wgpu to 0.6 in iced_wgpu #500

Merged
merged 11 commits into from
Aug 31, 2020
Merged

Update wgpu to 0.6 in iced_wgpu #500

merged 11 commits into from
Aug 31, 2020

Conversation

hecrj
Copy link
Member

@hecrj hecrj commented Aug 27, 2020

Updates iced_wgpu to use the latest release of wgpu 🎉

The new release of wgpu features a new StagingBelt type that we can leverage to reduce reallocations of staging buffers. As a consequence, we no longer pin gfx-memory to an old version and, therefore, iced_wgpu should no longer leak video memory!

The integration example has also been updated accordingly.

Fixes #438 and closes #486.

@hecrj hecrj added bug Something isn't working improvement An internal improvement labels Aug 27, 2020
@hecrj hecrj added this to the 0.2.0 milestone Aug 27, 2020
@hecrj hecrj self-assigned this Aug 27, 2020
@hecrj hecrj mentioned this pull request Aug 27, 2020
@kvark
Copy link

kvark commented Aug 27, 2020

Did you consider write_buffer/write_texture instead of the staging belt?

examples/integration/src/scene.rs Outdated Show resolved Hide resolved
wgpu/src/image.rs Outdated Show resolved Hide resolved
wgpu/src/backend.rs Show resolved Hide resolved
wgpu/src/image.rs Outdated Show resolved Hide resolved
wgpu/src/image.rs Outdated Show resolved Hide resolved
wgpu/src/quad.rs Outdated Show resolved Hide resolved
wgpu/src/triangle.rs Outdated Show resolved Hide resolved
wgpu/src/triangle.rs Outdated Show resolved Hide resolved
wgpu/src/triangle.rs Outdated Show resolved Hide resolved
wgpu/src/triangle.rs Outdated Show resolved Hide resolved
@hecrj
Copy link
Member Author

hecrj commented Aug 27, 2020

@kvark Hey, thanks for the review!

Did you consider write_buffer/write_texture instead of the staging belt?

Yes, but we are kind of stuck with a StagingBelt and multiple render passes until we can lift some limitations in wgpu_glyph (see hecrj/wgpu_glyph#46). Once those are dealt with, we will change the architecture to upload everything upfront and record all the draw calls in a single render pass.

While we could partially use write_buffer/write_texture right now, I think it's better to batch all the work once a full transition is possible. I plan to work on this very soon. This is just the first step!

@PolyMeilex
Copy link
Contributor

Great work, I updated my project to wgpu 6.0 and new iced and did some initial tests and everything works as expected so far 👌

But I have to say that as a user that integrates iced into existing wgpu project StagingBelt is quite annoying, I have to keep track of belt that I don't even use and I don't think I need one in my engine (as it is a quite simple one) I have to add it into my abstractions solely for use in wgpu_glyph and iced.
tldr: I'm glad you're considering to move away from StagingBelt in future, and I just wanted to voice my opinion 😃

@dhardy
Copy link

dhardy commented Aug 31, 2020

I agree with @PolyMeilex; still at least in KAS (kas-gui/kas#121) the StagingBelt doesn't leak into any of the examples, so it is less of an impact.

@hecrj hecrj merged commit ff15ebc into master Aug 31, 2020
@hecrj hecrj deleted the update-wgpu branch August 31, 2020 13:11
@hecrj
Copy link
Member Author

hecrj commented Aug 31, 2020

I'm glad you're considering to move away from StagingBelt in future, and I just wanted to voice my opinion

Once we can get rid of the StagingBelt, we should be able to also follow the middleware pattern.

still at least in KAS (kas-gui/kas#121) the StagingBelt doesn't leak into any of the examples, so it is less of an impact.

Glad to know! It doesn't leak into any of the iced examples either. iced_wgpu is the only subcrate directly leaking it, given that the user should be able to choose the chunk size.

@valpackett
Copy link
Contributor

grrr, another local futures executor, breaking my GLib based project again >_< patched to spawn on GLib for now. Hopefully the "move away from StagingBelt" would delete the executor.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working improvement An internal improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

update to wgpu 0.6 iced_wgpu should not pin gfx-memory to 0.1.1
5 participants