Avoiding unnecessary buffer copies

[dlight]

I submitted #104 to avoid copying the buffer every time I submit a texture to the gpu, but it turns out that each gpu backend receives a &image::DynamicImage and performs another copy anyway. (well one copy is better than two, but still)

on the gfx backend it always performs a copy (even if the image already is rgba)

on the wpu backend it always performs a copy too, even if it's already gbra (also: why do the two backends use a different color order for textures?)

For the opengl backend: I can't find it.

Well it's unavoidable to perform copies if the image isn't the right format, but it can be avoided if it is. But copying on some backends and not on others doesn't sound good, so I'm not sure if there's something that can be done.

(Anyway, my point of view is: the more optimized coffee is, the more people can get away with poorly optimized game code.)

[hecrj]

How often are you submitting a new texture to the GPU? You should try to reduce this as much as possible. Image::from_image is meant to be used for resource loading.

If you are producing individual pixels in your update logic, Coffee is probably not a good fit for your use case. Coffee allows you to load and draw sprites, quads, meshes, and UI widgets easily by leveraging the GPU. It is not optimized for drawing individual pixels. For something like that, you will probably have a better time with something like minifb.

If you are only loading resources every once in a while, I do not think an additional copy will matter unless you are dealing with a lot of images. We should probably perform some benchmarks (see #13) and profile it before optimizing anything, specially when there is not a clear way to go about it.

[dlight]

How often are you submitting a new texture to the GPU?

I don't know yet!

We should probably perform some benchmarks and profile it before optimizing anything, specially when there is not a clear way to go about it.

Fair enough.

[hecrj]

The wgpu backend has this line since the first release, which is quite ugly:

coffee/src/graphics/backend_wgpu/texture.rs

Line 259 in e1bb429

layers.iter().cloned().flatten().cloned().collect();

When I wrote this, I was just starting with Rust and I didn't focus much on performance, given that it's a method that should be used rarely, so there is definitely room for improvement/micro-optimization (see #7).

[dlight]

The last cloned is definitively unneeded, because collect already allocates a new memory buffer (so .cloned().collect() copies the stuff, then copies again to the collected container - in this case, Vec).

IIRC the first one (.iter().cloned()) is sometimes needed to workaround the borrow checker, but not this time, since you are collecting (thus you own the result)

Lol, I did test on the opengl backend, not vulkan

[dlight]

About graphics::Image::from_image: if we could ensure tha all backends always receive rgba buffers when uploading textures, then receiving

pub fn from_image(gpu: &mut Gpu, image: &image::RgbaImage) -> Result<Image>

Is probably a cleaner API: anyone that already have an RgbaImage can just pass it &image, and people that have some other kind of image shouldn't be too worried to call &image.convert() (which allocates an rgba buffer). It feels rusty, in the same way that having to call .clone() and .cloned() when needed is.