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

Texture atlas #154

Merged
merged 23 commits into from
Feb 28, 2020
Merged

Texture atlas #154

merged 23 commits into from
Feb 28, 2020

Conversation

Maldela
Copy link
Contributor

@Maldela Maldela commented Jan 12, 2020

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.

Copy link
Member

@hecrj hecrj left a comment

Choose a reason for hiding this comment

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

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!

wgpu/Cargo.toml Outdated Show resolved Hide resolved
wgpu/src/image.rs Outdated Show resolved Hide resolved
wgpu/src/image.rs Outdated Show resolved Hide resolved
wgpu/src/image.rs Outdated Show resolved Hide resolved
wgpu/src/image/raster.rs Outdated Show resolved Hide resolved
@hecrj hecrj added the improvement An internal improvement label Jan 13, 2020
@hecrj hecrj mentioned this pull request Jan 15, 2020
@Maldela
Copy link
Contributor Author

Maldela commented Jan 17, 2020

Sorry it took me so long to update the PR. I think the new one addresses all your requests.

@hecrj hecrj mentioned this pull request Jan 18, 2020
@hecrj
Copy link
Member

hecrj commented Jan 18, 2020

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! 🥂

@Maldela
Copy link
Contributor Author

Maldela commented Feb 10, 2020

Just rebased on current master. Go ahead and rename or move anything you like.

@hecrj hecrj added this to the 0.1.0 milestone Feb 19, 2020
Copy link
Member

@hecrj hecrj left a comment

Choose a reason for hiding this comment

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

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,
Copy link
Member

Choose a reason for hiding this comment

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

I changed this to Uint to make passing non-sensical values as the layer index impossible.

});

self.texture_version = texture_version;
}
Copy link
Member

Choose a reason for hiding this comment

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

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,
);
Copy link
Member

Choose a reason for hiding this comment

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

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);
}
}
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@Maldela Maldela Feb 27, 2020

Choose a reason for hiding this comment

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

Wasn't sure about that one either. Automatic deallocation just seemed kind of "rusty".

Atlas {
texture,
texture_view,
layers: vec![Layer::Empty, Layer::Empty],
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

@hecrj hecrj Feb 27, 2020

Choose a reason for hiding this comment

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

It's 2048 currently, we could maybe lower it more.

Copy link
Member

Choose a reason for hiding this comment

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

In the long run, wgpu should offer a way to check available VRAM so we can choose a proper value at runtime.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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;
Copy link
Member

Choose a reason for hiding this comment

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

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();
}
}
Copy link
Member

@hecrj hecrj Feb 26, 2020

Choose a reason for hiding this comment

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

Overall, I really like the simplicity of this module.

@Maldela
Copy link
Contributor Author

Maldela commented Feb 27, 2020

If you're happy with it, then I'm happy with it as well.

@hecrj
Copy link
Member

hecrj commented Feb 28, 2020

I have changed the renderer so it doesn't create a useless image::Pipeline (and therefore, an empty atlas) when both image and svg flags are disabled.

I think this ship is ready to sail. Thank you for all the work here! 🎉

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

Successfully merging this pull request may close these issues.

2 participants