OpenDiablo2 / HellSpawner

The OpenDiablo2 toolset
GNU General Public License v3.0
152 stars 45 forks source link

remove `giu.Texture` example; use image.RGBA instead #333

Open gucio321 opened 3 years ago

gucio321 commented 3 years ago

Hi there, we can remove/reduce texture loading stuff as it isn't really necessary. for images (in dcc/dc6/dt1 editors) we can use ImageWithRGBA instead of Image. at the moment, no idea for image buttons, I opened an issue: AllenDang/giu#233 the final step, could be removing TextureLoader as it'll not be necessary anymore.

gucio321 commented 3 years ago

I can do that.

gravestench commented 3 years ago

From your description above, I'm not sure what the issue is.

What is the issue?

gucio321 commented 3 years ago

hmm, okey there is few issues, i see with texture loader.

  1. everyone, who wants to use our widgets, needs to pass TextureLoader there, I don't think it is ok, just like: why user should care, how the widget loads its textures?
  2. it isn't necessary to load textures with textureLoader for images (in dcc/dc6/dt1 editors). we're creating image.RGBA and then, converting to giu.Texture. but at the moment giu has ImageWithRgbaWidget so we don't need to do that.

so, in summary, there is a lot of code, we can clean up and simplify.

gravestench commented 3 years ago

But you're actually proposing multiple refactors:

1) Remove hscommon.TextureLoader from widget provider functions (this kind of detail should be hidden from external callers, makes sense) 2) Change image implementation used by widgets (to reduce complexity, good idea) 3) Remove hscommon.TextureLoader entirely from codebase (make loading textures less painful, I like it)

If this is the case, I'd like for you to close this issue and open individual issues, and to provide coherent titles and descriptions for those issues.

Thanks!

gucio321 commented 3 years ago

yah, thats the case, but these points are strongly related with each other. you can't Remove hscommon.TextureLoader from widget provider functions before you Change image implementation used by widgets (images and imageButtons btw). if you wish, i can open as separated issues, but we will still have to implement them in a specific order.

gravestench commented 3 years ago

you can't Remove hscommon.TextureLoader from widget provider functions before you Change image implementation used by widgets

I'd appreciate it if you provided that kind of detail when you open an issue.

Also, it sounds like that wouldn't be an issue if we did another refactor that unifies the way we load images throughout the entire codebase (I thought this is what hscommon.TextureLoader was doing.)

gucio321 commented 3 years ago

okey, after a few tries, I came to the conclusion that removing giu.Textures isn't so easy. the reason is: imageWithRGBAWidget has bug/feature that causes losing image texture when image isn't visible. So in dcc/dc6 editors we'll se a kind of delay while scrolling around frames/directions. It'd be better to preload textures and show them by ImageWidget

in case of TextureLoader: I'm not sure it is necessary. Does anyone remember why don't we use giu.NewTextureFromRgba(...)?