egui 0.16

[Jengamon]

Basically copied egui_glium's Painter for the new 0.16 API, as TextureAllocator no longer provides a mutable self.

Right now it doesn't nicely extract epi::backend::TexAllocationData from an epi::Frame nor does it handle it, which could be something to add.

[Jengamon]

Def not the best way to do this (it's really easy to step on your own feet). Anyone have any ideas?

[parasyte]

Just a casual onlooker. Included some thoughts below!

[parasyte]

Shouldn't the id be selected for the caller? It's an internal implementation detail. The actual id allocated is already included in the return value, which should be good enough? I can't think of any cases where I would want to specify the id when creating a texture.

[parasyte]

Let's also remove this and just offer the other free_... method.

[parasyte]

And delete all this commented code.

[hasenbanck]

Thank you for your PR.

This PR seem to implement more than just adding support for egui 0.16. Please rebase you changes and only add only the changes that are needed for the current feature set. For example: I'm not sure why you arced the BindGroup. "set_texture()" seems to be a new feature?

I also fell that the trait for egui_texture_to_wgpu() is overkill and we could simply use parameters.

The cleanup suggestions by parasyte also seem valid.

[parasyte]

My understanding is that the set_texture thing is trying to follow emilk/egui#999

I don't know if changing texture allocation is necessary, here. But if I read that PR right, the high-level API appears to be in epi:

• https://github.com/emilk/egui/blob/0960f385523e4206a23c92f1dbb06330cb9ce02a/epi/src/lib.rs#L333-L341
• https://github.com/emilk/egui/blob/0960f385523e4206a23c92f1dbb06330cb9ce02a/epi/src/lib.rs#L344-L352

This is what I would suggest looking to for inspiration.

[Jengamon]

I also have a discussion here, but my big problem is that TextureAllocator is now by shared reference, which makes it impractical for the TextureAllocator to be the Renderpass. (I'm probably wrong, but the only way I can think of off the top of my head is to make Renderpass send messages to itself? Might work idk.)

I also fell that the trait for egui_texture_to_wgpu() is overkill and we could simply use parameters.

Yeah, it is... I did it so that epi::FontImage and a custom image data type can both work with the method without duplication

For example: I'm not sure why you arced the BindGroup.

I was basically following https://github.com/emilk/egui/pull/999/files#diff-23f23e44bac11e24b5797a2fa0734dbcd65b21830492cde1ad030e2314e76e7f and here the Texture points to an Rc, which hints that the type should be clonable because of how epi::NativeTexture works: it takes the texture by move, so if users want to also display the texture outside of egui, they still need a copy, which explains the Arc. Honestly, it should be Arc<wgpu::Texture>.

"set_texture()" seems to be a new feature?

Not really, I moved what TextureAllocator used to do here, because we can't mutate from TextureAllocator anymore.

So action items:

• Remove Arc<BindGroup>, and replace with Arc<Texture> in NativeTexture someone with more experience than me should figure out how to deal with native textures.
• Remove Arc from user_textures
• Remove free_texture and rename free_texture_id -> free_texture
• Make set_texture not take an input id
• Extract data directly from epi::backend::TexAllocationData (so that epi::Frame can still work with us, but I'm not sure how to handle this) [do I provide a method like extract_frame_data? idk anymore]
• Use message passing to pass stuff from uses as TextureAllocator into a pending queue too many assumptions, just abandoning TextureAllocator for now.
• Clean up

[parasyte]

but my big problem is that TextureAllocator is now by shared reference

If that's the only problem, you can use a type that offers interior mutability. RefCell or Mutex, for instance. It looks like you started to do that with the commented code, but abandoned it for some reason?

It should also be mentioned that this crate doesn't use epi at all, except to implement the TextureAllocator trait.

[Jengamon]

If that's the only problem, you can use a type that offers interior mutability. RefCell or Mutex, for instance. It looks like you started to do that with the commented code, but abandoned it for some reason?

I tried that, but it got real nasty, as this crate returns a reference to the bind groups it holds internally, but you reasonably can't send a reference through a lock, the lock doesn't live long enough... (and bind groups aren't Clone, so no go there).

[Jengamon]

It should also be mentioned that this crate doesn't use epi at all, except to implement the TextureAllocator trait.

That's a pretty important use (to me), as it was what made egui_example work at all!

[Jengamon]

I also fell that the trait for egui_texture_to_wgpu() is overkill and we could simply use parameters.

Does this mean handling UserTexture and epi::FontImage separately? Wouldn't that duplicate work? I might be misunderstanding what you are saying...

[parasyte]

I tried that, but it got real nasty, as this crate returns a reference to the bind groups it holds internally, but you reasonably can't send a reference through a lock, the lock doesn't live long enough... (and bind groups aren't Clone, so no go there).

Got it. I'll be honest, I haven't really dug into the issue or tried to work through it in practice. But then again #52 proposes to remove the TextureAllocator implementation entirely.

That's a pretty important use (to me), as it was what made egui_example work at all!

Yes, but I'm not sure you need to use epi for anything other than the egui_demo_app, or another app that is already built on it. If starting from scratch, you don't need it. The whole thing is kind of confusing, which is why I opened emilk/egui#212. Emil's first response is spot on.

In light of that, I don't think there is a need for epi or its TextureAllocator trait at all.

[hasenbanck]

I just pushed a version that support egui 0.16 to crates.io. If something is missing, please open another issue / PR.