Skip to content
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

feat(scale): set tick drawing order #6185

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from
Draft

Conversation

C47D
Copy link
Contributor

@C47D C47D commented May 6, 2024

Description of the feature or fix

Be able to change the drawing order of the tick widget

Notes

@C47D
Copy link
Contributor Author

C47D commented May 6, 2024

Note: Not final revision but it seems to be working as expected. The existing render tests are passing.

@kisvegabor
Copy link
Member

kisvegabor commented May 6, 2024

Thank you! Just please add a test for draw_ticks_on_top = 1.

@C47D
Copy link
Contributor Author

C47D commented May 7, 2024

I took the code snippet of the previous pull request and made the scale bigger (in the original size the minor ticks look weird)

We can notice that the sections doesn't exactly end in the major ticks, this is expected (unless somebody else implemented in) because when drawing arcs I wasn't able to find a way to increase the arc angle depending on the section tick widths.

image

@C47D
Copy link
Contributor Author

C47D commented May 7, 2024

When setting the scale as horizontal the sections are gone, this happens also in master
image

Example scale 2 is OK
image

@kisvegabor
Copy link
Member

We can notice that the sections doesn't exactly end in the major ticks, this is expected (unless somebody else implemented in) because when drawing arcs I wasn't able to find a way to increase the arc angle depending on the section tick widths.

The error at 18 seems quite significant. I'll take a look later. Hopefully we will find a way to get closer to the tick.

When setting the scale as horizontal the sections are gone, this happens also in master

If it's a regression we should have a test to cover it in the future.

@C47D
Copy link
Contributor Author

C47D commented May 8, 2024

I will check the sample code later today, I only copied and pasted it as is to run the examples.

@C47D
Copy link
Contributor Author

C47D commented May 8, 2024

@kisvegabor Do you know a way to calculate the angle between the arc center and a given point?

@liamHowatt
Copy link
Collaborator

Try lv_atan2. lv_arc uses it here. The x and y are the point minus the arc center.

@i400s
Copy link

i400s commented May 9, 2024

I've been playing around and stepping through code after making a couple of minor changes to my meter (as in the original lv_example_scale_7 example I created) and I've noticed a couple of things.

I boosted the size of the meter to (500, 500) and first changed the green section to (40, 48) and the red section to (76, 98) and ran the program. This was to make sure that the green should fall on full ticks, and that at least one section fell on some minor ticks.

As you can see the arcs are very much out of place.
Screenshot_2024-05-09_15-15-00

The sections gave section angles of 135-159, 256-280, and 365-405 respectively.

I then changed the scale by a multiplier of 8 to (0, 705) and also changed the sections values to match the new scale Blue (0, 64) Green (320, 384) and Red (608, 705) while leaving the ticks at (89).

The sections are a closer match... which is to be expected as there are now more "radial points" (for want of a better description) compressed in to the scale, 704 vs 89.

Screenshot_2024-05-09_15-32-00

The sections gave section angles of 135-159, 257-282, and 367-405.

Those 1 and 2 degrees difference are the killer!

Now having more scale is not an issue as such because it does have the advantage of allowing for greater accuracy for say the needle; or if the needle is required to only move by minor ticks then simply incrementing the count by multiples of 8 fixes that.

That said, having more scale does not correlate exactly to more unique points of movement. In theory I should now have 8 sub points per minor tick but due to a mix of integer maths in lv_scale_set_line_needle_value for the line: angle = scale->angle_range * (value - scale->range_min) / (scale->range_max - scale->range_min); and (possibly) the fixed 360 point angle maths (via an array) in lv_trigo_sin I end up with 3 or 4 (or maybe 5, I didn't check every ticks sub-ticks) actual positions even though there is enough "room" between ticks for more sub-ticks (at least with such a large scale) as seen in the zoomed in picture below. (The major tick is 3 pix and there is room for roughly 6 of them, meaning about 18 px of movement.)

For example the angle calculation (simplified):

270*319/705 = 122.170 = 122
270*320/705 = 122.553 = 122
270*321/705 = 122.986 = 122

Which means 3 sub-ticks all calculate to the same degree value.

Screenshot_2024-05-09_10-19-04

Now at this point my mathematical skills start to fall down...

I initially thought the issue might be resolved by using radians and de-coupling the degrees to the range of the scale but then after lots of calculations with 2π / 360 and 2π / (scale/(270/360)) with different scales I realized that when I did some conversions between the degrees unit and the scales unit the values were off when they should have been exactly the same (or very close) but I found that sometimes it was out by somewhere around 1 to 2 degrees.

I think the actual issue is that the scale requires a +1 when it should not have a +1. A major scale starts on 0 and then for each major scale tick the minor tick scale starts at 1.

M1234567M1234567M and so on. This means that for scale of 88 there should be 88 ticks of which 0, 8, 16, 24, 32, 40, 48, 56, 64, 72, 80, 88 are major ticks but because there is no 89 it doesn't major tick 88.

If there were 87 ticks then 80 would be a major, but the scale should stop on 87 (a minor tick) however as there is no 88 tick it doesn't draw the minor tick. Note there is only 6 minor ticks shown below when there should be 7.

Screenshot_2024-05-09_20-26-36

This means that to get the ticks to draw the ticks are calculated along a line that has one more item, so when it comes to calculating the section radius its out by 1 which means the angle calculations are off by that 1 element of scale. So when the scale is upped to 705 (should really be 704) its still off by 1 but that 1 is a much smaller part of the angle of arc.

So if I'm correct (I may not be) then there should be no +1 for the scale... just as there is no +1 for degrees (360 stops exactly on the 0; it doesn't require 361).

@kisvegabor
Copy link
Member

Your detailed description is very useful. I'll have a busy Friday and weekend but I will give it a go on Monday.

@i400s
Copy link

i400s commented May 11, 2024

Hi @kisvegabor, I have made more progress with the issue of the ticks/arcs misalignment.

I don't know how to apply the 4 commit changes that @C47D made to my local repository (I tried git apply from the .patch files pulled from github, but it kept failing with errors; I'm sure someone with a lot more git experience would know how to fix the issue).

However, I have managed to get the arcs and the ticks to align much better and without the current +1 requirement of the lv_scale_set_range. via changes to two lines (Note, the same changes would probably need to be applied to the straight line scale).

Using:

lv_scale_set_total_tick_count(scale, 87);
lv_scale_set_major_tick_every(scale, 8);
lv_scale_set_range(scale, 0, 87);

...

lv_scale_section_set_range(section_red, 76, 87);

This is the output:
Screenshot_2024-05-11_18-58-48

Using:

lv_scale_set_total_tick_count(scale, 87);
lv_scale_set_major_tick_every(scale, 8);
lv_scale_set_range(scale, 0, 696);

...

lv_scale_section_set_range(section_blue, 0, 64);

...

lv_scale_section_set_range(section_green, 320, 384);

...

lv_scale_section_set_range(section_red, 608, 696);

The output is:

Screenshot_2024-05-11_19-16-10

The changes are:

diff --git a/src/widgets/scale/lv_scale.c b/src/widgets/scale/lv_scale.c
index 627ea2777..a513ea25e 100644
--- a/src/widgets/scale/lv_scale.c
+++ b/src/widgets/scale/lv_scale.c
@@ -617,7 +617,7 @@ static void scale_draw_indicator(lv_obj_t * obj, lv_event_t * event)
         uint32_t label_gap = LV_SCALE_DEFAULT_LABEL_GAP; /* TODO: Add to style properties */
         uint32_t tick_idx = 0;
         uint32_t major_tick_idx = 0;
-        for(tick_idx = 0; tick_idx < scale->total_tick_count; tick_idx++) {
+        for(tick_idx = 0; tick_idx <= scale->total_tick_count; tick_idx++) {
             /* A major tick is the one which has a label in it */
             bool is_major_tick = false;
             if(tick_idx % scale->major_tick_every == 0) is_major_tick = true;
@@ -1034,7 +1034,7 @@ static void scale_get_tick_points(lv_obj_t * obj, const uint32_t tick_idx, bool
         center_point.x = scale_area.x1 + radius_edge;
         center_point.y = scale_area.y1 + radius_edge;
 
-        int32_t angle_upscale = ((tick_idx * scale->angle_range) * 10U) / (scale->total_tick_count - 1);
+        int32_t angle_upscale = ((tick_idx * scale->angle_range) * 10U) / (scale->total_tick_count);
         angle_upscale += scale->rotation * 10U;
 
         /* Draw a little bit longer lines to be sure the mask will clip them correctly

I have not looked into the arc drawing yet, just the tick part so far (I'm still very slow with c), but its so close that I'm quite buzzing with excitement at having got this far.

Its interesting that the alignment of the arc seems to be off in exactly the same place and amount for the blue and green.

Also interesting is that the major tick at "20" isn't green, as it should be because 6*64=384. That the two screen caps now match much more closely with different range scales is hopefully a positive step. I did find that to get the first major green tick to go grey requires a value of 324 and to get the grey major tick to turn green requires 388. This suggests the issue is possibly similar to the needle issue (accuracy) or there is maybe some offsetting done. If I get some time tomorrow afternoon (I'm on UK time) after dropping the granddaughter off I'll have a look into section colour code and also the arc code.

@FASTSHIFT FASTSHIFT changed the title feat(scale): Set tick drawing order feat(scale): set tick drawing order May 12, 2024
@i400s
Copy link

i400s commented May 12, 2024

I have now resolved the issue of the ticks which are now drawing correctly for all examples and changing colour at the correct points. The reason it didn't before is because the calls to lv_map() were subtracting 1 which obviously threw the calculations out.

The changes have been made to both the straight edge scale calculations and the arc edge scale calculations so that they no longer require +1's for the ticks and/or the ranges.

A tick count of 10 means 0-10 ticks.
A range of 0 to 100 means 100 elements.
Combining the above means 0-100 sub divisions with ticks at 0, 10, 20, 30 ... 100.

The changes however required a change to the examples to make them work. Obviously this was because they were still adding a +1 to the total tick count. This is obviously a breaking API change so I don't know how you would want to handle that, or if such a change is a serious NO.

As I wasn't working on the changes with @C47D changes applied I've created a diff of the changes.

The coloured sweeping arcs are still off slightly but I felt that maybe the two issues should be dealt with in two different patches.

I've attached the diff patch file (in a zip as github doesn't seem to like attaching .patch files or I've not created a valid patch file!).

diff.zip

I hope this helps and is acceptable.

@i400s
Copy link

i400s commented May 13, 2024

Having finally worked out how to apply the patch files for the initial 4 commits in this PR to a clean lvgl repository and then applying my patch to that I can see there is a bug when testing all the lv_example_scale_* examples.
Screenshot_2024-05-13_12-52-03

The reason for this was due to the scale_calculate_main_compensation() not being amended to account for there no longer being a +1 so was working on one tick short in its processing.

After the fix the result is correct.
Screenshot_2024-05-13_13-20-00

All the other examples seem to render correctly and the ticks above/below seems to work as intended.

patch-b1ba1897f8faf25ef0f3469c5a7801b79e940e8f.zip

@C47D
Copy link
Contributor Author

C47D commented May 14, 2024

Hi @i400s thanks for the investigation, I had a long weekend so I'm just reading your findings.

So the main issue seems to be that the tick count considers the initial tick (tick 0 let's call it) into the tick count? If so I think that's the expected behavior, is that correct @kisvegabor ? If we change it it might break all of the usages of the scale out there I think.

@i400s
Copy link

i400s commented May 14, 2024

Hi @C47D. Due to your post (the edited version) I've had another look at the code and I think my changes are completely superfluous to requirements.

If on my repository I performed a git reset --soft HEAD~ and git stash to remove all the changes I made. I then amended my scale to have a tick count of 88 and a range of 0-696 instead of 0-704 it all seems to align correctly with the arcs and ticks (although the arc is still slightly out).

The alignment/tick colour issues I was having was down to talking my original meter code and just transplanting it with the scale code and not completely understanding the link between ticks and range. The +1 requirement of the ticks (which then gets subtracted in the lv_scale code) lead me to think that the range also needed to some how be off by some variation of +1 or (number of ticks * multiplier) +1 which then obviously meant the alignments were off which I was then trying to correct by tweaking the sections instead of fixing the error with the base scale range.

My changes, it seems, are completely redundant which is somewhat embarrassing (although it has allowed me to get a better grip on the code). Which means there is no need to break the API.

I guessing my misunderstanding was probably quite unique due to the way I wanted my scale to have sub-ticks for fractions of degrees (so I could move the needle with greater precision than the minor ticks).

@kisvegabor
Copy link
Member

Wow, what an interesting outcome!

If the current method is working as it is, we at least need t describe it in the docs (maybe in the functions' API comments too) and add a dedicated example. As you understand much better than me what is happening, could you add these?

@i400s
Copy link

i400s commented May 14, 2024

If we change the code to have the following:

lv_scale_set_total_tick_count(scale, 89);
lv_scale_set_major_tick_every(scale, 8);
lv_scale_set_range(scale, 32, 384); // Greater range allows more precision, 4 sub-ticks, for the movement of needle.
lv_scale_set_draw_ticks_on_top(scale, true);
...
lv_scale_section_set_range(section_blue, 32, 64);
...
lv_scale_section_set_range(section_green, 192, 224);
...
lv_scale_section_set_range(section_red, 336,384);
...
lv_anim_set_values(&anim_scale_line, 32, 384);

and remove the custom_labels[] then we are left with the following visual:
Screenshot_2024-05-14_13-00-17

This makes the link between ticks and range visually much clearer and this shows the ticks_on_top (which is what this was meant to be all about).

For the ticks count being 88 + 0th = 89 ticks I think that was a really daft mistake on my part.

That I didn't see that the example_scale_4 0-100 temp scale was 21 ticks including the 0th element and the range actually starts on the zero was my brain completely missing the obvious (and compounded by the code comment mentioning what happens if there is a tick count of 1 which would cause a div 0 error).

I'm not sure how one would word the API/text to say the ticks count should include the first element but the range already takes that in to account. For the examples with a range of 0 to something, the zero is right there. For this example the range relation to the number of tics should hopefully be much more obvious with it not starting on zero!

@C47D
Copy link
Contributor Author

C47D commented May 15, 2024

Hi @C47D. Due to your post (the edited version) I've had another look at the code and I think my changes are completely superfluous to requirements.

If on my repository I performed a git reset --soft HEAD~ and git stash to remove all the changes I made. I then amended my scale to have a tick count of 88 and a range of 0-696 instead of 0-704 it all seems to align correctly with the arcs and ticks (although the arc is still slightly out).

The alignment/tick colour issues I was having was down to talking my original meter code and just transplanting it with the scale code and not completely understanding the link between ticks and range. The +1 requirement of the ticks (which then gets subtracted in the lv_scale code) lead me to think that the range also needed to some how be off by some variation of +1 or (number of ticks * multiplier) +1 which then obviously meant the alignments were off which I was then trying to correct by tweaking the sections instead of fixing the error with the base scale range.

My changes, it seems, are completely redundant which is somewhat embarrassing (although it has allowed me to get a better grip on the code). Which means there is no need to break the API.

I guessing my misunderstanding was probably quite unique due to the way I wanted my scale to have sub-ticks for fractions of degrees (so I could move the needle with greater precision than the minor ticks).

Don't worry, I felt the same when coming back to the scale code, it's been a while since I first did it. I'll check the code to find the divisions and try to make it less prone to rounding errors.

I'm not sure how one would word the API/text to say the ticks count should include the first element but the range already takes that in to account. For the examples with a range of 0 to something, the zero is right there. For this example the range relation to the number of tics should hopefully be much more obvious with it not starting on zero!

Do you have any suggestions to improve the docs @kisvegabor ?

@kisvegabor
Copy link
Member

Do you have any suggestions to improve the docs @kisvegabor ?

I think we should add 2 things:

  1. In the Sections section add practical example to describe a real life case to map values to ticks and explain why it's not trivial. If possible dding an equation would be nice too.
  2. Add an lv_example_scale to demonstrate it.

@i400s
Copy link

i400s commented May 15, 2024

I've created an example you may find useful.

Eventually my intention is to use it, and an ESP32C6, to control my central heating via a simple relay replacing a rather old timer/temp switch.

Screenshot_2024-05-15_15-26-20

Actual result on a 2.9" device in dark mode (currently on a breadboard):
IMG_20240515_163138172_HDR

The LED switches on/off as the needle crosses a temperature.

It may need some cleaning up in that I prefix statics with an s_

But it does use calculations to work out the scales/ranges and the interaction between them (including on the temperature bands and the led switching).

Zip file containing 1 header, and 1 c file:
app_ui.zip

@C47D
Copy link
Contributor Author

C47D commented May 20, 2024

Hi, I was in a rather busy week at work, will have some free time next Wednesday to take a look at this 😸

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants