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

Use StagingBelt for uploading data #46

Merged
merged 1 commit into from
Aug 25, 2020
Merged

Use StagingBelt for uploading data #46

merged 1 commit into from
Aug 25, 2020

Conversation

hecrj
Copy link
Owner

@hecrj hecrj commented Aug 22, 2020

Instead of reallocating staging buffers every frame, we ask the user to provide a staging belt of buffers.

@rukai @dhardy Does this seem like a good idea to you?

Instead of reallocating staging buffers every frame, we ask the user to provide a staging belt of buffers.
@hecrj hecrj added the enhancement New feature or request label Aug 22, 2020
@hecrj hecrj added this to the 0.10.0 milestone Aug 22, 2020
@rukai
Copy link
Contributor

rukai commented Aug 22, 2020

I have never used the staging belt myself and the extra boilerplate needed to use wgpu_glyph didn't look very appealing.
So I asked cwfitzgerald what he thought about it and he said:
image
image

@cwfitzgerald
Copy link

To add on to that, when I ported imgui-wgpu over to 0.6, I used write_buffer and was able to keep the api consistent with the middleware pattern. I need to do proper benches comparing write_buffer with a staging belt at some point, but I'm not sure exactly how to properly bench it.

@hecrj
Copy link
Owner Author

hecrj commented Aug 22, 2020

The StagingBelt is replacing a bunch of Device::create_buffer_init and CommandEncoder::copy_buffer_to_buffer.

We cannot easily use Queue::write_buffer because glyph-brush forces us to couple both buffer preparation and drawing (the caching strategy can overwrite old glyphs). In other words, if a user wants to draw two different layers of text in the same submission, we need different buffers for each layer. Therefore, we need composability at the CommandEncoder level.

I will remove the layer limitation eventually, but it is not the point of this PR.

@dhardy
Copy link

dhardy commented Aug 22, 2020

The glyph-brush caching strategy doesn't precisely meet kas-text's needs: for that, ahead-of-time buffer preparation would be possible (although it would require a few changes). Probably for any application, clearing of unused glyphs could be demoted to an infrequent garbage-collection cycle (or disabled entirely — e.g. many games will have a hard limit on the number of glyphs they actually draw).

Regarding the specifics of how to copy data to GPU memory I'm the wrong person to ask.

@cwfitzgerald
Copy link

Oh I see the problem, that's a tough one.

Given those constraints, I think this is reasonable until that constraint can be lifted.

@hecrj hecrj merged commit 7143398 into master Aug 25, 2020
@hecrj hecrj deleted the feature/staging-belt branch August 25, 2020 08:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants