Add New GPU Texture Backend for advanced Usages (see paint example !)

[cjbrigato]

Work in progress, do not merge. Fixe many image related things ( complexe use of state, widgets bloat, GPU Resources Leaks, etc)

loadimageAdvanced example

loadimageAdvanced.webm

new AsyncImage example

asyncimage.webm

new "paint" basic example

paintexample.webm

[gucio321]

@cjbrigato how is it going?

[cjbrigato]

I have to rebase on master then to pass the whole linter. If you can wait ~12h it'll be done, but it will still lack a bit of documentation and the "OnFailure" Layout that was part of the old ImageWithURL widget

[gucio321]

no problem, we can wait :smile:

[cjbrigato]

@gucio321 oh come on

go 1.22

toolchain go1.23.1

you are mean with me XD. I really wasn't ready for 1.23 toolchain. Anyway, I'm diving in !

[gucio321]

@cjbrigato just a protip: you could just git merge upstream/master then you need to solve conflicts only once ;-)

[cjbrigato]

@gucio321 the PR is ready. Added new example "asyncimage", demonstrating the ease of use of "Stateful" version of the ReflectiveBoundTexture, alongside with "pure imgui" windows with noOS Decoration. Also demonstrate the "file:///" loading scheme and "SurfaceLoaders" usage. This should all be really easy to extend, in GIU or at end user level. This do not use the whole and complicated widget state system in context. Hope you don't dislike too much this PR, I know it is strongly opinionated at some point, but it feel quite robust, and puts the GPU Bound resources bugs far away.

Thanks in advance !

And keep up the amazing work ! Full Documentation and explanation are to come in next PR.

[cjbrigato]

(properly rebased so review is possible)

[cjbrigato]

It looks cool, but i have some more points:

• could you just replace the old imagewithrgba with your new implementation (or implemen it there somehow), I think the current version of an image is still broke but could be easily fixed now
• i'd opt for adding a missing doc comments already here (tbh i have no ideas why golqngci passes without them)

Just don't feel like you are under pressure to finish this asap. It's an oss project and we work here when we want t and have time 😁

Actually i'm using it in an important project, so the pressure does not come from giu but from the project context ;)

made the two little changed you asked for, will update documentation and will make the StatefullReflective underlying object of ImageWithRGBA/ImageWithURL :)

[cjbrigato]

@gucio321 problem with "ImageWithRGBA" is the way itself it's designed: Even with my solution, which ensure unbindings, you have to "keep" the object somewhere as to manage it's state and to ensure unbinding. You cannot count on finalizer at all there. Problem is that ImageWithRGBA and subsequent binds textures then "forget" about GPU Resources. Using my new object result in the same behavior if we don't either Unbind or keep track of an instanciated object.

I deeply advice against the current design of the ImageWith* object right now, an encourage users to keep track of objects that BIND resources to GPU, which is the case for textures.

We can "hide" such object management on GIU side, and it will work... but for the very case of Texture I still think this is a very bad choice, hence the provided "loadimage" example changes.

I'll provide a change to "ImageWithRGBA" which demonstrate "hidden management" of the GPU Resources binding but again I strongly advice not to use it as is. The design is very well thought for other objects indeed (labels,buttons, etc) but these are not GPU Bound object like resources, so this is absolutely not comparable.

see: https://github.com/AllenDang/giu/pull/855#issuecomment-2377975991

[cjbrigato]

@gucio321 I have a proposal being inbetween, the old "ImageWithX" and the user fully managed state.

I pushed some Methods to ReflectiveBoundTexture that result in only needing keeping the ReflectiveBoundTexture on user side before usage, then is simple as the old Widget usage:

This would work and be best of both world. I've already prepared so it works with "ImageWithRGBA", "WithFile" and "WithURL" methods, all returning an ImageWidget already

See: https://github.com/AllenDang/giu/pull/855/commits/d1485b141311adcff60b301c450f735bbddf885d

PS: users already have to managed their own owbject when using InputTXT, DragInt etc so I see no reason why they would not do the same with Textures.

Edit: By Just writing the previous line I think I know exactly what is the proper design to propose :) Hold on

See: https://github.com/AllenDang/giu/pull/855#issuecomment-2377975991

[cjbrigato]

I got it !

@gucio321 loadimage become loadimageadvanced example asyncimage is also there but the pre-PR loadimage example is BACK with no modification in design except that the user should provide a pointer to the backend object in the very same way a user should provide a pointer to string, bool, int, float whatever for other RW widgets !

See: https://github.com/AllenDang/giu/pull/855/files#diff-549290e4ff1313d18f9138dbb7fa403cea00f702d0e681ed50219cbb643431a2

I think this is quite the right track :)

Note the legacy loadimage example takes max of 22MB footprint in GPUResources now :D

Now, back to linting, documentation AND I spoted a little forgotten object: the ImageButton widget.

[cjbrigato]

@gucio321 Fixed ImageButtonWithRGBA too, changed a comment on ImageWidget since the simplification Deeply rely on it and it's simplicity as an underlying built object and container, and also added full documentation.

Considering my previous comment on design which bring backs nearly unmodified loadimage example, and the provided documentation, I think this PR is ready for a final review :)

[cjbrigato]

Added minimalist but insightfull "paint" example to demonstrate more ReflectiveBoundTexture usage (see https://github.com/AllenDang/giu/pull/855#issue-2544664033)

[cjbrigato]

the linter just doest catch anything under the example directory for me.

I'll do the requested Changes, but i'm more than reluctant for the first one, as explained. But you got a deal if I add a setter :D

[gucio321]

i'm more than reluctant for the first one, as explained. But you got a deal if I add a setter :D

You know, the plain user of giu who just wwants to "Here is my RGBA and I want it to appear!" doesn't need *ReflectiveBoundTexture in his programm. He just wants it to work - and thats a purpose of giu :smile:

but the pre-PR loadimage example is BACK with no modification in design except that the user should provide a pointer to the backend object in the very same way a user should provide a pointer to string, bool, int, float whatever for other RW widgets !

I think getter/setter for Image should be enough. If someone really needs it then it is there :smile:

[cjbrigato]

Collaborator

i'm more than reluctant for the first one, as explained. But you got a deal if I add a setter :D

You know, the plain user of giu who just wwants to "Here is my RGBA and I want it to appear!" doesn't need *ReflectiveBoundTexture in his programm. He just wants it to work - and thats a purpose of giu 😄

but the pre-PR loadimage example is BACK with no modification in design except that the user should provide a pointer to the backend object in the very same way a user should provide a pointer to string, bool, int, float whatever for other RW widgets !

I think getter/setter for Image should be enough. If someone really needs it then it is there 😄

That's impressive to be capable of both the whole cimgui-go machinery and still have that capacity of providing a rapid and simple framework on top of it like it's done there. I find it quite impressive, I definitely haven't easily this ability to switch and keep from my opinionated idioms. Will definitely handle the underlying texture backend in a state and add a getter.

[gucio321]

wow, I just ran paint. Its awesome!!!

[gucio321]

@cjbrigato could you use go:embed in paint example? Then we can just say: look what we have in giu go run github.com/AllenDang/giu/examples/paint@latest

[cjbrigato]

@gucio321 yes i'll use embed, provide the missing save/open undo and brush sizing func too. I specifically took < 64px icons with embed in mind After I made the changes you asked for. Glad you like it, I literally made it to kill time while waiting for review

[cjbrigato]

@gucio321 do you have any "quick" implementation on puting the &ReflectiveBoundTexture{} into your giu.Context state machinery ? It's being quite a PR and when I started looking at GetState[T any, PT genericDisposable[T]](c *context, id string) PT {, even If I love Generics, if I can avoid diving in too much I'd be glad. Also I've never seen doubling a SyncMap usage with mutexes, since the whole point of the syncMap is to avoir having to handle mutexes.

[gucio321]

@gucio321 do you have any "quick" implementation on puting the &ReflectiveBoundTexture{} into your giu.Context state machinery ?

yeah, there was one already but it wasn't perfect I think. Let me search a sec...

e.g. here: https://github.com/gucio321/yamler/blob/master/pkg/widget/state.go

type State struct {
   // anything you want here
}

func (s *State) Dispose() {
    // noting to do here
}

func (w *Widget) GetState() *State {
    if s := giu.GetState[State](giu.Context, w.stateID()); s != nil {
        return s
    }

    newState := w.newState()
    giu.SetState(giu.Context, w.stateID(), newState)

    return w.GetState()
}

func (w *Widget) newState() *State {
    return &State{
                // initialize here
        }
}

Edit And you need stateID (in case of image it should be (*ImageWidget).id or something like that

[gucio321]

@cjbrigato just wanted to warn you: I suppose something is broken inside giu and State is disposed every frame (?) I'm investigating right now

[cjbrigato]

That would explain part of initial imagewithrgba bug. Yes the finalizer was an issue but there definitely was something more going on

[gucio321]

That would explain part of initial imagewithrgba bug. Yes the finalizer was an issue but there definitely was something more going on

Yes and yes ;-)

I think that also that sync should be investigated some day.

btw, I've added this state example to FAQ :smile_cat: https://github.com/AllenDang/giu/wiki/FAQ#state

[cjbrigato]

Good catch. When do you merge it so I can rebase ?

[cjbrigato]

I won't have time to work on this this weekend and it still lack the state for the widget and for some reason I'm struggling with having a dedicated surfaceLoader working with embed.FS (for paint) but I'll get back to it on monday

[gucio321]

@cjbrigato I see you updating paint example ;-)

Glad you like it, I literally made it to kill time while waiting for review

Does it mean I should review?

[cjbrigato]

@gucio321 I just could not resist providing brush size and undo buffer to the paint example so it's done. I'll get back to the state machinery tommorow but in the meantime* do we have an ETA on linting example directories ?

I don't think you should review before I do the requested changes on state and ViewportConfig too

[gucio321]

do we have an ETA on linting example directories ?

I have an answer from golangci how to enable it but it'd rquire me to fix a ton of isues on current code :smile:

BTW Do you want me to merge #866 now or wait for this (it may be a small breaking change but i can fix it on my PR later)?

[cjbrigato]

do we have an ETA on linting example directories ?

I have an answer from golangci how to enable it but it'd rquire me to fix a ton of isues on current code 😄

BTW Do you want me to merge #866 now or wait for this (it may be a small breaking change but i can fix it on my PR later)?

Yes please merge, i'll rebase and fix quickly tommorow then handle the state !

[gucio321]

@cjbrigato now linting examples works. I'm going to reenable revive linter to enforce correct documentation but idk when I'll be able to do that as giu still misses some docs.

BTW, sorry for change in ImageWidget.go

[cjbrigato]

@gucio321 I tought your changes on state would fix original but same problem as ever, GPU filling memory in under 3minutes.

So.. i'll rebase and retry my luck on imagewidget but putting the reflectiveTexture in state

[cjbrigato]

@gucio321 fixed ImageWithRGBA in https://github.com/AllenDang/giu/pull/869

This PR now is renamed and serve other purpose ("advance" usage for power users ;) )

[cjbrigato]

I'm closing this in favor of new pr, because of the changes in master