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

fix(draw): fix the invalidation of scaled areas #5548

Merged
merged 12 commits into from Mar 4, 2024

Conversation

kisvegabor
Copy link
Member

Description of the feature or fix

fix #5447

Notes

@kisvegabor
Copy link
Member Author

cc @lhdjply

@lhdjply
Copy link
Contributor

lhdjply commented Feb 1, 2024

I tested it. It's an improvement over before. However, there is still some residue after the bezel is scaled.
For some reason, I can't upload files larger than 10MB on github, so I'll just use the images this time.You can try the test code yourself to see the actual results.
Screenshot_20240201_210010_com huawei browser

test code

static void anim_cb(void * obj, int32_t value)
{
  lv_obj_set_style_transform_scale(obj, value, LV_PART_MAIN);
}

void test_demo(void)
{
  lv_obj_t *obj=lv_obj_create(lv_screen_active());
  lv_obj_set_size(obj,200,200);
  lv_obj_set_style_bg_color(obj,lv_palette_main(LV_PALETTE_BLUE),LV_PART_MAIN);
  lv_obj_set_style_transform_pivot_x(obj, 100, LV_PART_MAIN);
  lv_obj_set_style_transform_pivot_y(obj, 100, LV_PART_MAIN);
  lv_anim_t aa;
  lv_anim_init(&aa);
  lv_anim_set_var(&aa, obj);
  lv_anim_set_values(&aa, 0, 256);
  lv_anim_set_time(&aa, 500);
  lv_anim_set_playback_time(&aa, 500);
  lv_anim_set_repeat_count(&aa, LV_ANIM_REPEAT_INFINITE);
  lv_anim_set_path_cb(&aa, lv_anim_path_linear);
  lv_anim_set_exec_cb(&aa, anim_cb);
  lv_anim_start(&aa);
}

@kisvegabor
Copy link
Member Author

Oh, thank you! I'm on it.

@PGNetHun PGNetHun changed the title fix(draw): fix the invaldation of scaled areas fix(draw): fix the invalidation of scaled areas Feb 4, 2024
@kisvegabor
Copy link
Member Author

Huh, it seems working. To be honest I'm not so happy with it because it's hacky here and there.

The current image transformation algorithm has some precision issues due to the integer math. I have some ideas to improve it, but it will require much more time.

@lhdjply
Copy link
Contributor

lhdjply commented Feb 6, 2024

I think we can merge first to fix the bug, and then slowly step by step, optimize the code, through the accuracy.

@IAMMX
Copy link
Contributor

IAMMX commented Feb 6, 2024

I tested it. It doesn't seem to fix it. It's still the original result.

Original picture

The image format is ARGB8888.
295810594-7354024f-896b-4cec-8ac0-f4f58ecc85b8

Code

LV_IMG_DECLARE(img_test);
void img_scale_test(void)
{
  lv_obj_t *logo1;
  logo1 = lv_image_create(lv_screen_active());
  lv_image_set_src(logo1, &img_test);
  lv_obj_align(logo1,LV_ALIGN_CENTER,-200,0);
  lv_image_set_scale(logo1, 256);

  lv_obj_t *logo2;
  logo2 = lv_image_create(lv_screen_active());
  lv_image_set_src(logo2, &img_test);
  lv_obj_align(logo2,LV_ALIGN_CENTER,200,0);
  lv_image_set_scale(logo2, 255);
}

Result

Screenshot_20240206_131321_com huawei browser

@kisvegabor
Copy link
Member Author

@IAMMX
I've debugged it an it turned out that it's a rounding error related to LVGL stores some coordinates internally with 1 px resolution. This is what happens:

  • The image has 240px width, meaning x1 = 0, x2 = 239
  • When we scale it down to 255/256 around pivot_x = 120 we get x1 = 0.5 -> 0; x2 = 118.5 -> 238.
  • So the scaled image is on x1=0; x2= 238;
  • When LVGL calculates the inverse scale during transformations, (with pivot_x = 120 )it will get x2 = (238-120) * 256 / 255 + 120 = 238.4. It's stored with higher precision, be we should have get 239 here
  • So the precision was lost when we stored the transformed area with 1 px precision

Probably it can be improved, but it would be larger scale update.

As a workaround you can do 2 things:

  • add a few transparent pixel around the image
  • calling lv_image_set_pivot(logo, 2, 2) also solved the issue for me in this particular config as this way we do the calculations with 239 instead of 119. (meaning less error)

@IAMMX
Copy link
Contributor

IAMMX commented Feb 8, 2024

add a few transparent pixel around the image

This method can work, but every time you create the image, be careful to leave some transparent areas.

calling lv_image_set_pivot(logo, 2, 2) also solved the issue for me in this particular config as this way we do the calculations with 239 instead of 119. (meaning less error)

This method will not work if it is used for picture scaling animations.

@kisvegabor
Copy link
Member Author

lv_image_set_pivot(logo, 2, 2)

It doesn't change anything, but I meant lv_image_set_pivot(logo, 0, 0)


Unfortunately I don't have a good solution for the accuracy issue now. 🙁

@lhdjply
Copy link
Contributor

lhdjply commented Feb 11, 2024

In 4dcd66e, the image layer_transform_2.png is added, but in the code, no TEST_ASSERT_EQUAL_SCREENSHOT("draw/layer_transform_2.png") is found;

@kisvegabor
Copy link
Member Author

True, thank you. Removed.

@kisvegabor
Copy link
Member Author

@PGNetHun could you review this PR?

Copy link
Collaborator

@PGNetHun PGNetHun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some minor naming related comments.
I've not tested changes, but see that reference images are updated, so I assume code is OK.

Bonus test case (optional):
Can you please add also 1 test:
show a frame of a sprite/texture atlas, and rotate it.
I use sprites in my app (using lv_image offset_x, offset_y, and rotation), and the sprite is shown wrong (cut of image is after rotation, so parts of next frame appear, too).
Example:
animation of ants: use a sprite PNG as source for lv_img and lv_animation (+ callback to set image offset_y) for frame animation, and rotate the lv_img to the walking direction.
ant

The bug:
image

src/core/lv_obj_pos.h Outdated Show resolved Hide resolved
src/core/lv_obj_pos.c Show resolved Hide resolved
src/misc/lv_area.h Outdated Show resolved Hide resolved
@kisvegabor
Copy link
Member Author

Can you please add also 1 test:
show a frame of a sprite/texture atlas, and rotate it.

Well, I haven't realized it so far, but there is no such a feature in LVGL now. Always the whole image is rotated and the clip area of the destination area is just a normal (not rotated) rectangle described by 2 points. To support what you have described we needed to apply a clip area on the source image too. It's possible but not supported now.

As workaround you can transform the image as a layer:

    lv_obj_set_style_transform_rotation(img1, 450, 0);
    lv_obj_set_style_transform_pivot_x(img1, 0, 0);
    lv_obj_set_style_transform_pivot_y(img1, 0, 0);

It's slower and uses more memory, but for small images it shouldn't be a big issue.

@IAMMX
Copy link
Contributor

IAMMX commented Feb 16, 2024

Screenshot_20240216_150522_com huawei browser

Why did you delete all these test pictures?

I think that's the reason #5664

@PGNetHun
Copy link
Collaborator

Can you please add also 1 test:
show a frame of a sprite/texture atlas, and rotate it.

Well, I haven't realized it so far, but there is no such a feature in LVGL now. Always the whole image is rotated and the clip area of the destination area is just a normal (not rotated) rectangle described by 2 points. To support what you have described we needed to apply a clip area on the source image too. It's possible but not supported now.

I have realized this "issue" only after migrating from v8.3 to v9.
Thanks for the suggested code, I will try it.

PGNetHun
PGNetHun previously approved these changes Feb 16, 2024
@kisvegabor
Copy link
Member Author

Thank you for pointing it out. I've just added back the reference images images.

@IAMMX
Copy link
Contributor

IAMMX commented Feb 20, 2024

Since you have already merged master, please remove aeb60a9. Because the issue has been fixed in 46e338e.

@kisvegabor
Copy link
Member Author

I have reverted it. We will squash commits so don't be scared off the long history 🙂

@lhdjply
Copy link
Contributor

lhdjply commented Feb 24, 2024

This branch has conflicts that must be resolved

@kisvegabor
Copy link
Member Author

Fixed

@kisvegabor kisvegabor requested review from lhdjply, XuNeo and FASTSHIFT and removed request for lhdjply February 24, 2024 13:53
src/core/lv_obj_pos.h Outdated Show resolved Hide resolved
examples/widgets/image/lv_example_image_1.c Outdated Show resolved Hide resolved
src/core/lv_obj_pos.c Show resolved Hide resolved
src/core/lv_obj_pos.c Show resolved Hide resolved
@kisvegabor
Copy link
Member Author

Thank you, Neo. All fixed.

@FASTSHIFT FASTSHIFT merged commit ac51808 into lvgl:master Mar 4, 2024
16 checks passed
@kisvegabor kisvegabor deleted the fix/transform branch March 4, 2024 05:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
7 participants