ImpulseAdventure / GUIslice

GUIslice drag & drop embedded GUI in C for touchscreen TFT on Arduino, Raspberry Pi, ARM, ESP8266 / ESP32 / M5stack using Adafruit-GFX / TFT_eSPI / UTFT / SDL
https://www.impulseadventure.com/elec/guislice-gui.html
MIT License
1.17k stars 209 forks source link

Area of refresh is much larger than the element that is being refreshed (region invalidation?) #328

Closed MickTheMechanic closed 3 years ago

MickTheMechanic commented 3 years ago

Describe the bug

*When a GUI element is redrawn (for example a button that is pressed or a text field that is updated) the area that is redrawn is much larger than the element that needs redrawing. This is particularly inconvenient with multiple elements on a page, multiple are redrawn when only one changes.

Text input and Number input are not affected if fill is enabled*

Device hardware

Expected behavior

if an element needs to be redrawn (for example a text element is updated with a new value) only this element should be redrawn.

Is there some way to localize or reduce region invalidation?

ImpulseAdventure commented 3 years ago

Please note that I have been very busy at work over the past few weeks, so unfortunately I have not been able to address questions in the repository in a timely manner. Thanks for your patience!

Hi @MickTheMechanic --

Thank you for filing this... For reference, this looks like it was originated in #325

What you are seeing is a manifestation of multiple elements being redrawn within a single "Update" call.

This has come up before in some other user cases, so I have been giving some thought as to ways to make this more optimized for this common use-case.

Approach 1: Dual invalidation regions One approach to optimizing the case where a button deselction (for example) triggers other updates would be to maintain two regions. Within a single update cycle:

I think the above might work to make the common case a lot better visually.

Approach 2: Separate the callbacks from the GUI updates Alternately, I could see if there is a way to separate out the redraw updates caused by a button release to be completed before we invoke the callback functions. This could be the cleanest solution of all, but I'll need to take a careful look at the code to see how I might "save" the pending callbacks until a future Update() cycle.

In other words, the first call to Update() would handle the user element interaction (eg. button deselection), and then it would be the next Update() pass that triggers the callbacks (and any GUI updates they trigger).

If anyone has any thoughts on an approach to take, I'm certainly open to ideas!

thanks cal

MickTheMechanic commented 3 years ago

Thanks for the reply. I have since noticed that it only seems to affect elements with no background, which is also mentioned in the documentation.

The problem seems to be the worst when dealing with image buttons that pull from SD. The problem becomes worse when muliple image buttons are present on the same page.

I wonder if enabling background for image buttons could help in any way? Would it enable a localised redraw, the same way enabling fill on a text field does?

Pconti31 commented 3 years ago

@ImpulseAdventure It would be helpful if you outlined your algorithm. For example, your code in GUIslice.c clearly shows you building up the invalidation region but other than setting a clipping region it doesn't appear to be used. How exactly are you using this information and how is it triggering the background redraws we see? What routines are involved? Paul--

Pconti31 commented 3 years ago

@ImpulseAdventure Nevermind. I figured out how it works.

  if (bPageRedraw) {
    gslc_DrvDrawBkgnd(pGui);
    gslc_PageFlipSet(pGui,true);
  }

By setting clip region to a smaller area the background is drawn to the size of the invalidation region. What had confused me was the comments around the name of the variable bPageRedraw which implied it would only be set for a full page redraw while in fact its set inside gslc_PageRedrawCalc() in either case. Paul--

ImpulseAdventure commented 3 years ago

That’s right.... the PageRedrawCalc() looks for certain conditions that trigger the need for a deeper refresh beyond whatever element was updated. Conditions where the background may be revealed (eg. Updates to transparent elements, making an element hidden or changing the page) require us to walk through a full redraw.

I could provide much more advanced logic to assess what elements need redrawing (based on Z order) but it can get complex fast.

Instead, I trigger a full “page redraw” but then leverage the clipping region (invalidation rect) to determine what will ultimately be visible. If an element doesn’t appear in the region then it won’t need redrawing. Therefore, the redraw can be fairly localized to the invalidation region.

Of course some display drivers don’t support clipping regions, so we take advantage of it when we can.

The original issue posted here was a case wherein the invalidation region was larger than it theoretically needs to be because of concurrent updates.

My idea of splitting the updates into the GUI redraw and the callbacks seems like it might work seamlessly, but I haven’t had a good look into the code yet.

thanks

ImpulseAdventure commented 3 years ago

Finally had an opportunity to revisit this issue and start working on the implementation...

It looks like the cleanest approach will be to defer the callback function triggers until after the initial button redraws are done. This way if a button press/release triggers another GUI visual change, these will be split into separate redraw cycles, preventing the need to grow the refresh region needlessly.

ImpulseAdventure commented 3 years ago

I have now uploaded a fix for the "large refresh area". It supports the creation of a "pending redraw" state that allows controls to be redrawn before any of the side-effect redraws. The net result should be a much smaller redraw region.

Please try out the following branch:

Example:

ImpulseAdventure commented 3 years ago

Feature now merged. If further enhancements are required for this redraw scenario, please feel free to open a new issue. thanks!