fix(buttonmatrix): skip buttons outside clip area in draw_main#9946
fix(buttonmatrix): skip buttons outside clip area in draw_main#9946andrewleech wants to merge 2 commits intolvgl:masterfrom
Conversation
|
Hi 👋, thank you for your PR! We've run benchmarks in an emulated environment. Here are the results: ARM Emulated 32b - lv_conf_perf32b
Detailed Results Per Scene
ARM Emulated 64b - lv_conf_perf64b
Detailed Results Per Scene
Disclaimer: These benchmarks were run in an emulated environment using QEMU with instruction counting mode. 🤖 This comment was automatically generated by a bot. |
There was a problem hiding this comment.
1 issue found across 1 file
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/widgets/buttonmatrix/lv_buttonmatrix.c">
<violation number="1" location="src/widgets/buttonmatrix/lv_buttonmatrix.c:689">
P2: Clip culling uses the base button area before popover expansion, so popover-only intersections can be skipped and render stale/missing visuals.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
3df5e60 to
8b08c0e
Compare
LVGL submodule pinned to andrewleech/lvgl fix/buttonmatrix-clip-area-skip branch which adds clip area check in buttonmatrix draw_main() to skip off-screen buttons before style resolution. PR: lvgl/lvgl#9946. LV_DEF_REFR_PERIOD reduced from 33ms to 16ms. LV_USE_SYSMON enabled for FPS/CPU% perf monitor overlay.
There was a problem hiding this comment.
Pull request overview
Optimizes lv_buttonmatrix rendering by skipping per-button style resolution and draw task creation for buttons that do not intersect the current layer clip area.
Changes:
- Adds a per-button clip intersection check in
draw_main()to early-continue for off-clip buttons. - Extends the clip test area upward for popover-capable buttons to account for popover drawing.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| *Account for popover which extends the button area upward by its height.*/ | ||
| { | ||
| lv_area_t clip_test = btn_area; | ||
| if(btnm->ctrl_bits[btn_i] & LV_BUTTONMATRIX_CTRL_POPOVER) { | ||
| clip_test.y1 -= lv_area_get_height(&btn_area); | ||
| } |
There was a problem hiding this comment.
The new clip-area early-continue tests only the base btn_area (plus a popover height adjustment), but the button’s actual rendered footprint can extend outside btn_area due to outline/shadow/etc. (see invalidate_button_area() in this file, which explicitly enlarges invalidation because buttons “might have outline and shadow”). If the layer clip area intersects only that extended region (e.g., within the gaps used for shadow/outline invalidation), this guard can skip the button entirely and leave stale pixels in the invalidated area.
Consider expanding clip_test by the same conservative margin used for invalidation (row/col gaps with the dpi/10 minimum, or a max of them on both axes), and ideally apply the popover extension only when it will actually be drawn (pressed + popover).
| *Account for popover which extends the button area upward by its height.*/ | |
| { | |
| lv_area_t clip_test = btn_area; | |
| if(btnm->ctrl_bits[btn_i] & LV_BUTTONMATRIX_CTRL_POPOVER) { | |
| clip_test.y1 -= lv_area_get_height(&btn_area); | |
| } | |
| *Expand the test area conservatively because the rendered footprint can extend | |
| *outside the base button area due to outline/shadow/etc. | |
| *Account for popover only when it will actually be drawn.*/ | |
| { | |
| int32_t row_gap = lv_obj_get_style_pad_row(obj, LV_PART_ITEMS); | |
| int32_t col_gap = lv_obj_get_style_pad_column(obj, LV_PART_ITEMS); | |
| int32_t extra_row = LV_MAX(row_gap, BTN_EXTRA_CLICK_AREA_MAX); | |
| int32_t extra_col = LV_MAX(col_gap, BTN_EXTRA_CLICK_AREA_MAX); | |
| int32_t clip_margin = LV_MAX(extra_row, extra_col); | |
| lv_area_t clip_test = btn_area; | |
| clip_test.x1 -= clip_margin; | |
| clip_test.y1 -= clip_margin; | |
| clip_test.x2 += clip_margin; | |
| clip_test.y2 += clip_margin; | |
| if((btnm->ctrl_bits[btn_i] & LV_BUTTONMATRIX_CTRL_POPOVER) && (btn_state & LV_STATE_PRESSED)) { | |
| clip_test.y1 -= lv_area_get_height(&btn_area); | |
| } |
There was a problem hiding this comment.
Addressed in the latest force-push. The clip test area now extends by clip_margin (computed once before the loop from pad_row/pad_column with dpi/10 minimum, matching invalidate_button_area). This ensures buttons whose shadow/outline extends into the gap area are not incorrectly skipped.
The popover extension remains unconditional on the flag (not gated on pressed state) because invalidate_button_area also extends unconditionally — the invalidated region for a popover button includes the popover area regardless of current press state.
8b08c0e to
c4daf8a
Compare
draw_main iterates all buttons unconditionally, performing style resolution and draw dispatch even for buttons outside the clip area. Add an lv_area_is_on() check to skip buttons that don't intersect the layer's clip area. The test area is extended by a conservative uniform margin (max of row and column gaps, with dpi/10 floor) to account for outline and shadow rendering outside the base button area. The popover flag additionally extends the test area upward by the button's height. Once a button's top edge is below the clip area bottom, all remaining buttons are skipped via early break (buttons are laid out top to bottom). Signed-off-by: Andrew Leech <andrew.leech@planetinnovation.com.au>
Create a buttonmatrix inside a small container and scroll it so the top and bottom rows are clipped. Verifies that the clip-area skip optimization in draw_main does not incorrectly hide visible buttons at the edges of the clipped region. Signed-off-by: Andrew Leech <andrew.leech@planetinnovation.com.au>
c4daf8a to
35a80d9
Compare
|
Please check failing tests |
Description
draw_main()inlv_buttonmatrix.citerates every non-hidden button unconditionally. For each button it performs style resolution (lv_obj_init_draw_rect_dsc,lv_obj_init_draw_label_dsc), text measurement, and draw task dispatch. The lower-level draw functions clip pixels correctly, but the per-button style walks and draw task creation still execute for buttons entirely outside the dirty region.This adds an
lv_area_is_on()check after computing each button's screen-space area. Buttons that don't intersectlayer->_clip_areaare skipped before any style resolution or draw calls.The fix is a single
continueguard -- no structural changes, no new allocations, no API changes.Motivation
When a single button changes state (e.g. highlight on press), LVGL invalidates only that button's area. The refresh system correctly limits the clip area to the dirty region. However,
draw_mainstill walks all buttons, performing full style resolution for each one. On a widget with many buttons, most of this work is discarded.Test setup and results
Hardware: Cortex-M7 @ 996 MHz, 720x1280 display, MIPI-DSI, double-buffered direct-mode rendering, via lv_binding_micropython.
Widget: 40-button keyboard (4 rows of 10 keys), styled with custom theme (borders, backgrounds, font rendering).
Methodology:
time.ticks_us()aroundlv_timer_handler()calls after programmatically changingbtn_id_selto trigger a single-button invalidation. 20 sequential key transitions measured.The "distant button" case is slower before the fix because two invalidation regions (old and new button) span a wider clip area, causing more rows of buttons to fall inside the clip region and receive full style processing. After the fix, only buttons actually intersecting the clip area are processed regardless of distance.