-
-
Notifications
You must be signed in to change notification settings - Fork 138
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
Add New GPU Texture Backend for advanced Usages (see paint example !) #855
Conversation
e8ae07d
to
83f4858
Compare
@cjbrigato how is it going? |
I have to rebase on master then to pass the whole linter. |
no problem, we can wait 😄 |
@gucio321 oh come on
you are mean with me XD. I really wasn't ready for 1.23 toolchain. Anyway, I'm diving in ! |
@cjbrigato just a protip: you could just |
@gucio321 the PR is ready. Thanks in advance ! And keep up the amazing work ! |
cleaning Add ImageV/ImageButton casting from structure, remove pprof in loadimages _ add setsurfacesn forceCOmmit We are nearly done there. Fix Onclick widget with Size consideration, Add Scale operation in imageWidget, implement SurfaceLoaders for easy extension of loading images, change loading image example for advanced drawings and controls, updates URL downloaded images. Last Step: URL With OnError/OnLoading to mimic old behavior with better extension. Linting, Documentation.
* permits statefull state and async operation (goroutines) * keep state in mainloop, or be reset if needed * load is async and takes SurfaceLoader interface * onReset, onLoading, onFailure, onSuccess callbacks * Kept simple as ever, can go to an ImageWidget * or can be used cleverly in a customWidget Incoming loadimage exemple
Featuring the whole StatefullReflectiveBoundTexture. This quite lacks documentation but I'll do that in next PR.
4084279
to
b791da3
Compare
(properly rebased so review is possible) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 :) |
@gucio321 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.
see: #855 (comment) |
@gucio321 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.
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: #855 (comment) |
They are now compatible with legacy loadimage, which is also back ! now provide loadimageAdvanced and asyncimage example for complex examples The only new need of legacy ImageWidget usage is that you provide a pointer to ReflectiveBoundTexture for WithRGBA/File a pointer to StatefulReflectiveBoundTexture for WithURL due to its async nature and the custom layout perStates. This should not be hard to manage it's the very same as providing pointer to string for InputText, or pointer to any objet for other widgets !
** I got it !**@gucio321 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. |
@gucio321 |
@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 |
yeah, there was one already but it wasn't perfect I think. 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 |
@cjbrigato just wanted to warn you: I suppose something is broken inside giu and State is disposed every frame (?) |
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 btw, I've added this state example to FAQ 😸 |
Good catch. |
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 |
@cjbrigato I see you updating paint example ;-)
Does it mean I should review? |
@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 |
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 ! |
@cjbrigato now linting examples works. BTW, sorry for change in |
@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 |
I'm closing this in favor of new pr, because of the changes in master |
Work in progress, do not merge.Fixe many image related things ( complexe use of state, widgets bloat, GPU Resources Leaks, etc)
loadimageAdvanced example
loadimage.webm
new AsyncImage example
asyncimagesend.webm
new "paint" basic example
paintexample.webm