Texture atlas#154
Conversation
There was a problem hiding this comment.
Looks like a decent start! Thank you for giving it a shot :)
There are some things that need more work before we can merge, though! Currently, we are not getting any benefits from using an atlas.
We should also have in mind that a texture cannot keep growing indefinitely. Different hardware has different limits. To be on the safe side, we should assume atlases cannot have a dimension bigger than 8192.
Therefore, we will need to leverage multiple atlases (maybe texture arrays or arrays of textures could help us!) and even split huge images in multiple smaller ones. I am not saying we should implement this on a first iteration, but it's something we should keep in mind when designing the current data model.
Remember you can reach out to me on Zulip to discuss design ideas and ask any questions. When it comes to involved features like this one, I think this kind of interaction is crucial to help me understand your approach, get on the same page, avoid wasting efforts, and finally get your code merged. Code should be the last step. It's the easy part!
|
Sorry it took me so long to update the PR. I think the new one addresses all your requests. |
|
You actually went ahead and implemented a texture array approach! And it seems to even split big images in multiple allocations! Awesome! 🎉 There is a lot to unpack here, so it may take me a while to go over all the changes. I'd appreciate it if you could share some details about the general implementation and any challenges, tradeoffs, future improvements or anything in particular we should be aware of. How have you tested this? It would be great to create an example application to try these changes and, at the same time, showcase how the library can be used to create an actual application. Maybe some kind of image viewer? It may be a good idea to write some unit tests for the allocation logic. I just took a quick glance and I think I can take it to the finish line from here, if you don't mind of course. It should be mostly a matter of moving and renaming some stuff. Thanks for the amazing work! 🥂 |
|
Just rebased on current master. Go ahead and rename or move anything you like. |
- Split into multiple modules - Rename some concepts - Change API details
- Update texture view on grow - Fix atlas texture coordinates - Fix fragmented uploads
There was a problem hiding this comment.
I have finally gone through the changes and refactored some stuff! Sorry for the delay here 😅
I mostly changed names, moved code around, and simplified a bunch of things. So far, I am quite happy with the result. Let me know what you think!
We should be quite close to merging this 🎉
| }, | ||
| wgpu::VertexAttributeDescriptor { | ||
| shader_location: 5, | ||
| format: wgpu::VertexFormat::Uint, |
There was a problem hiding this comment.
I changed this to Uint to make passing non-sensical values as the layer index impossible.
| }); | ||
|
|
||
| self.texture_version = texture_version; | ||
| } |
There was a problem hiding this comment.
Not sure how expensive creating a bind group is, but now we are only re-creating it when necessary instead of every frame.
| &self.instances, | ||
| 0, | ||
| (amount * std::mem::size_of::<Instance>()) as u64, | ||
| ); |
There was a problem hiding this comment.
Recreating a vertex buffer every frame is expensive. Instead, we create one with a fixed size and reuse it every frame, uploading data from a staging buffer using offsets when needed.
| if let Memory::Device(entry) = memory { | ||
| atlas.remove(entry); | ||
| } | ||
| } |
There was a problem hiding this comment.
I reverted the smart deallocation on drop, as it added a bunch of Rc and RefCell complexity. I think we should keep deallocation centralized and easy to reason about.
There was a problem hiding this comment.
Wasn't sure about that one either. Automatic deallocation just seemed kind of "rusty".
| Atlas { | ||
| texture, | ||
| texture_view, | ||
| layers: vec![Layer::Empty, Layer::Empty], |
There was a problem hiding this comment.
Apparently, there is no way to create a texture array with an array_layer_count of 1. When we do this, we get a bunch of Vulkan validation errors because the created texture is a simple 2D one and the sampler is not compatible.
I am not sure if this is a limitation of the modern graphics backends or a wgpu design decision. We should probably bring it up in the wgpu matrix channel.
In any case, we need to start with at least 2 layers for now.
There was a problem hiding this comment.
Maybe we should decrease the atlas size then or make it configurable somehow. 4096 squared pixels times two is already 128 MB of data if I calculated that right.
There was a problem hiding this comment.
It's 2048 currently, we could maybe lower it more.
There was a problem hiding this comment.
In the long run, wgpu should offer a way to check available VRAM so we can choose a proper value at runtime.
There was a problem hiding this comment.
2048 is fine, I think.
| Entry::Fragmented { fragments, .. } => { | ||
| for fragment in fragments { | ||
| let (x, y) = fragment.position; | ||
| let offset = (y * width + x) as usize * 4; |
There was a problem hiding this comment.
I have simplified this a bunch. It turns out you can use copy_buffer_to_texture to upload a sub-region of an image quite easily.
| self.texture = new_texture; | ||
| self.texture_view = self.texture.create_default_view(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Overall, I really like the simplicity of this module.
|
If you're happy with it, then I'm happy with it as well. |
|
I have changed the renderer so it doesn't create a useless I think this ship is ready to sail. Thank you for all the work here! 🎉 |
I finally came around to implementing the texture atlas for images. I'm using guillotiere to manage the atlas space. In the current implementation the atlas only grows if necessary. It never shrinks.
I also wrote a small build script that compiles shaders at compile time using shaderc. Manually compiling shaders during development was super annoying.
Let me know what you think.