feat(chart): crowded-mode rendering on opengles support, with head/tail tint gradients#9938
Conversation
FishOfTheNorthStar
requested review from
AndreCostaaa,
FASTSHIFT and
kisvegabor
as code owners
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 3 files
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/chart/lv_chart.c">
<violation number="1" location="src/widgets/chart/lv_chart.c:1168">
P2: Scissor fill mode forces crowded_mode, which triggers the large points allocation and aborts rendering on OOM even though scissor fill never uses line_dsc.points. This introduces an unnecessary memory dependency and failure path for scissor fill rendering.</violation>
<violation number="2" location="src/widgets/chart/lv_chart.c:1243">
P2: Head/tail gradient distance is computed using `p_act` before it’s updated for the current loop index, so the tint/size effects are based on the previous point and shift by one sample.</violation>
</file>
<file name="src/widgets/chart/lv_chart.h">
<violation number="1" location="src/widgets/chart/lv_chart.h:31">
P2: LV_USE_CHART_SCISSOR_FILL_MODE is hardcoded in the public header after lv_conf_internal.h, overriding any project configuration and forcing this feature on for all builds. This removes configurability and can force unintended code paths to compile.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
|
||
| for(i = 0; i < chart->point_cnt; i++) { | ||
| if(next_none > -1) { | ||
| active_norm = (float)p_act / (float)(chart->point_cnt); |
There was a problem hiding this comment.
P2: Head/tail gradient distance is computed using p_act before it’s updated for the current loop index, so the tint/size effects are based on the previous point and shift by one sample.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/widgets/chart/lv_chart.c, line 1243:
<comment>Head/tail gradient distance is computed using `p_act` before it’s updated for the current loop index, so the tint/size effects are based on the previous point and shift by one sample.</comment>
<file context>
@@ -1093,7 +1215,35 @@ static void draw_series_line(lv_obj_t * obj, lv_layer_t * layer)
+
for(i = 0; i < chart->point_cnt; i++) {
+ if(next_none > -1) {
+ active_norm = (float)p_act / (float)(chart->point_cnt);
+ next_none_norm_dist = ((LV_ABS((next_none_norm - 1.f) - active_norm) < LV_ABS(next_none_norm - active_norm)) ?
+ (next_none_norm - 1.f) : next_none_norm) - active_norm;
</file context>
|
|
||
| /* Enable crowded mode rendering on Opengles output using | ||
| glScissor fills. Also enables head/tail tint gradients */ | ||
| #define LV_USE_CHART_SCISSOR_FILL_MODE 1 |
There was a problem hiding this comment.
P2: LV_USE_CHART_SCISSOR_FILL_MODE is hardcoded in the public header after lv_conf_internal.h, overriding any project configuration and forcing this feature on for all builds. This removes configurability and can force unintended code paths to compile.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/widgets/chart/lv_chart.h, line 31:
<comment>LV_USE_CHART_SCISSOR_FILL_MODE is hardcoded in the public header after lv_conf_internal.h, overriding any project configuration and forcing this feature on for all builds. This removes configurability and can force unintended code paths to compile.</comment>
<file context>
@@ -26,6 +26,10 @@ extern "C" {
+/* Enable crowded mode rendering on Opengles output using
+ glScissor fills. Also enables head/tail tint gradients */
+#define LV_USE_CHART_SCISSOR_FILL_MODE 1
+
/**********************
</file context>
| #define LV_USE_CHART_SCISSOR_FILL_MODE 1 | |
| #ifndef LV_USE_CHART_SCISSOR_FILL_MODE | |
| #define LV_USE_CHART_SCISSOR_FILL_MODE 1 | |
| #endif |
|
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 (changes from recent commits).
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/chart/lv_chart.c">
<violation number="1" location="src/widgets/chart/lv_chart.c:1235">
P2: Using `lv_value_precise_t` for head/tail normalization introduces integer-division behavior when `LV_USE_FLOAT=0`, breaking smooth crowded-mode gradient/tint transitions.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
You're iterating quickly on this pull request. To help protect your rate limits, cubic has paused automatic reviews on new pushes for now—when you're ready for another review, comment |
FASTSHIFT
changed the title
|
Can you provide Tests, Documentation, and Examples for new features? |
There was a problem hiding this comment.
Pull request overview
Adds an optional OpenGL ES–oriented “crowded mode” rendering path for lv_chart line series, along with configurable “head” and “tail” tint/size effects intended to highlight the newest/oldest data regions.
Changes:
- Introduces a new compile-time feature flag and public APIs for forcing crowded rendering and configuring head/tail effects.
- Implements an alternate crowded rendering path that uses per-column fill tasks (intended to map to
glScissor-style fills on OpenGL ES). - Extends the internal
lv_chart_tinstance data with head/tail and forced-crowded configuration fields.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
| src/widgets/chart/lv_chart.h | Adds a feature flag definition and public setter/getter APIs for crowded/head/tail behavior. |
| src/widgets/chart/lv_chart.c | Implements the new APIs and modifies line-series rendering to support the scissor-fill crowded mode with head/tail tinting. |
| src/widgets/chart/lv_chart_private.h | Adds new per-chart state fields (force crowded, head/tail params/colors) guarded by the new feature flag. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| glScissor fills. Also enables head/tail tint gradients */ | ||
| #define LV_USE_CHART_SCISSOR_FILL_MODE 1 |
There was a problem hiding this comment.
LV_USE_CHART_SCISSOR_FILL_MODE is being hard-defined to 1 in a public widget header, which forces the feature on for all builds (and for all render backends) and bypasses the normal LVGL configuration mechanism. This should be a user-configurable LV_USE_* option provided via lv_conf.h/Kconfig (and surfaced through lv_conf_internal.h), ideally defaulting to 0 and/or depending on LV_USE_DRAW_OPENGLES.
| glScissor fills. Also enables head/tail tint gradients */ | |
| #define LV_USE_CHART_SCISSOR_FILL_MODE 1 | |
| glScissor fills. Also enables head/tail tint gradients. | |
| * This option should normally come from lv_conf.h/lv_conf_internal.h. | |
| * Only provide a backend-aware fallback here when it is not configured. | |
| */ | |
| #ifndef LV_USE_CHART_SCISSOR_FILL_MODE | |
| #ifdef LV_USE_DRAW_OPENGLES | |
| #if LV_USE_DRAW_OPENGLES | |
| #define LV_USE_CHART_SCISSOR_FILL_MODE 1 | |
| #else | |
| #define LV_USE_CHART_SCISSOR_FILL_MODE 0 | |
| #endif | |
| #else | |
| #define LV_USE_CHART_SCISSOR_FILL_MODE 0 | |
| #endif | |
| #endif |
| * Set the tail power | ||
| * @param obj pointer to a chart object | ||
| * @param norm set a 0 to 4096 value representing how sharply the tail effect should taper in, 1024 = linear | ||
| */ | ||
| //void lv_chart_set_tail_pow(lv_obj_t * obj, uint32_t norm); | ||
|
|
||
| /** |
There was a problem hiding this comment.
There is a commented-out public API declaration (//void lv_chart_set_tail_pow(...)). Public headers shouldn’t contain dead/commented APIs because it becomes an accidental part of the interface and creates ambiguity for users. Either implement and document the API, or remove the commented declaration until it’s ready.
| * Set the tail power | |
| * @param obj pointer to a chart object | |
| * @param norm set a 0 to 4096 value representing how sharply the tail effect should taper in, 1024 = linear | |
| */ | |
| //void lv_chart_set_tail_pow(lv_obj_t * obj, uint32_t norm); | |
| /** |
| /** | ||
| * Get the tail color | ||
| * @param obj pointer to a chart object | ||
| * @param color set the color of the line at the tip of the tail | ||
| */ | ||
| void lv_chart_set_tail_color(lv_obj_t * obj, lv_color_t color); | ||
|
|
There was a problem hiding this comment.
The docstring for lv_chart_set_tail_color incorrectly says “Get the tail color” and has an extra @param color description under a “Get” heading. Update the comment to describe setting the tail color to match the function name and parameters.
| #if LV_USE_CHART_SCISSOR_FILL_MODE | ||
| bool force_crowded_mode; | ||
| uint32_t head_percent; | ||
| int32_t head_size_offset; | ||
| lv_color_t head_color; | ||
|
|
||
| uint32_t tail_percent; | ||
| int32_t tail_size_offset; | ||
| lv_color_t tail_color; | ||
| #endif |
There was a problem hiding this comment.
New fields are added under LV_USE_CHART_SCISSOR_FILL_MODE, but lv_chart_constructor doesn’t initialize them. They are later read during rendering (e.g., force_crowded_mode, head_percent, colors), which can lead to undefined behavior and non-deterministic output. Please add explicit defaults in the constructor when this feature is enabled.
| void lv_chart_set_head_percent(lv_obj_t * obj, uint32_t norm) | ||
| { | ||
| LV_ASSERT_OBJ(obj, MY_CLASS); | ||
|
|
||
| lv_chart_t * chart = (lv_chart_t *)obj; | ||
| if(chart->head_percent == norm) return; | ||
|
|
||
| chart->head_percent = norm; | ||
| lv_obj_invalidate(obj); |
There was a problem hiding this comment.
The setters accept normalized ranges documented as 0..1024 (head/tail percent), but they currently store the value without clamping/validation. This can produce unexpected behavior (e.g., >100% head/tail regions) and makes the API contract inaccurate. Consider clamping to the documented range (and documenting what happens at the extremes).
| coords.x1 = (int32_t)p_x; | ||
| coords.x2 = (int32_t)p_x + ex_size; | ||
| coords.y1 = (int32_t)y_min; | ||
| coords.y2 = (int32_t)y_max + ex_size; | ||
| if((int32_t)y_min == (int32_t)y_max) { | ||
| coords.y2 = (int32_t)y_min + 1; |
There was a problem hiding this comment.
ex_size can become <= 0 when head_size_offset/tail_size_offset are negative, which can produce invalid areas (x2 < x1/y2 < y1) passed to lv_draw_add_task. Also, lv_area_t coordinates are inclusive; coords.x2 = p_x + ex_size makes the filled width ex_size + 1. Clamp ex_size to at least 1 and compute x2/y2 using inclusive semantics (e.g., x2 = x1 + ex_size - 1).
| coords.x1 = (int32_t)p_x; | |
| coords.x2 = (int32_t)p_x + ex_size; | |
| coords.y1 = (int32_t)y_min; | |
| coords.y2 = (int32_t)y_max + ex_size; | |
| if((int32_t)y_min == (int32_t)y_max) { | |
| coords.y2 = (int32_t)y_min + 1; | |
| if(ex_size < 1) { | |
| ex_size = 1; | |
| } | |
| coords.x1 = (int32_t)p_x; | |
| coords.x2 = (int32_t)p_x + ex_size - 1; | |
| coords.y1 = (int32_t)y_min; | |
| coords.y2 = (int32_t)y_max + ex_size - 1; | |
| if((int32_t)y_min == (int32_t)y_max) { | |
| coords.y2 = (int32_t)y_min + ex_size - 1; |
| if(in_head) { | ||
| outcol = lv_color_mix(chart->head_color, outcol, (int8_t)(norm_edge_effect * 255.0)); | ||
| } | ||
| else if(in_tail) { | ||
| outcol = lv_color_mix(chart->tail_color, outcol, (int8_t)(norm_edge_effect * 255.0)); | ||
|
|
There was a problem hiding this comment.
lv_color_mix takes a uint8_t mix ratio (0..255), but the code casts to int8_t. Values >127 will wrap negative and lead to incorrect colors. Use an unsigned type (uint8_t/lv_opa_t) and clamp/round norm_edge_effect * 255 into [0,255] before calling lv_color_mix.
| if(in_head) { | |
| outcol = lv_color_mix(chart->head_color, outcol, (int8_t)(norm_edge_effect * 255.0)); | |
| } | |
| else if(in_tail) { | |
| outcol = lv_color_mix(chart->tail_color, outcol, (int8_t)(norm_edge_effect * 255.0)); | |
| if(in_head || in_tail) { | |
| lv_value_precise_t mix_value = norm_edge_effect * (lv_value_precise_t)255.0; | |
| if(mix_value < (lv_value_precise_t)0.0) { | |
| mix_value = (lv_value_precise_t)0.0; | |
| } | |
| else if(mix_value > (lv_value_precise_t)255.0) { | |
| mix_value = (lv_value_precise_t)255.0; | |
| } | |
| lv_opa_t mix = (lv_opa_t)(mix_value + (lv_value_precise_t)0.5); | |
| if(in_head) { | |
| outcol = lv_color_mix(chart->head_color, outcol, mix); | |
| } | |
| else { | |
| outcol = lv_color_mix(chart->tail_color, outcol, mix); | |
| } |
| } | ||
| #endif | ||
| bg_dsc->color = outcol; | ||
| bg_dsc->opa = LV_OPA_COVER; |
There was a problem hiding this comment.
In scissor-fill mode the fill descriptor forces bg_dsc->opa = LV_OPA_COVER, which ignores the chart’s line opacity/style (line_dsc.opa) and can change appearance compared to the normal path. Set the fill task opacity consistently with the line descriptor (and consider whether other style attributes like line width should be reflected too).
| bg_dsc->opa = LV_OPA_COVER; | |
| bg_dsc->opa = line_dsc.opa; |
This is in progress work and needs more testing before merging. I'm using github's code review and build tests to confirm this code works on it's own, outside of a larger use of it and other modified code in a demo. I'm carving just this part out of that into this PR.
This PR adds support for "crowded-mode" rendering of lv_chart's on OpenGL ES targets.
It also adds the idea of a head and tail of the data, with each having it's own normalized length and color.
