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(vector) : add path bounding and matrix transform functions. #5389

Merged
merged 5 commits into from Jan 23, 2024

Conversation

onecoolx
Copy link
Contributor

Help us review this PR! Anyone can approve it or request changes.

Description of the feature or fix

A clear and concise description of what the bug or new feature is.

Checkpoints

zhangjipeng added 3 commits January 19, 2024 10:04
Signed-off-by: zhangjipeng <zhangjipeng@xiaomi.com>
Signed-off-by: zhangjipeng <zhangjipeng@xiaomi.com>
Signed-off-by: zhangjipeng <zhangjipeng@xiaomi.com>
Comment on lines 324 to 325
for(uint32_t i = 1; i < len; i++) {
lv_fpoint_t * p = lv_array_at(&path->points, i);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of calling lv_arraya_at in the loop, which will always do sanity check, we can do this way since it's an actual array.

    lv_fpoint_t * p = lv_array_at(&path->points, 0);
    for(uint32_t i = 1; i < len; i++) {
        p[i].x = 

We can add a helper similar to STL vector to get the first element.
lv_array_front(&array);

https://en.cppreference.com/w/cpp/container/vector/front

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -195,6 +195,23 @@ void lv_matrix_multiply(lv_matrix_t * matrix, const lv_matrix_t * m)
_multiply_matrix(matrix, m);
}

void lv_matrix_transform_point(const lv_matrix_t * matrix, lv_fpoint_t * point)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
void lv_matrix_transform_point(const lv_matrix_t * matrix, lv_fpoint_t * point)
void lv_point_transform_by_matrix(lv_fpoint_t * point, const lv_matrix_t * matrix)

Copy link
Contributor Author

@onecoolx onecoolx Jan 19, 2024

Choose a reason for hiding this comment

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

I think it's weird that there is a single function called 'lv_point_xxxx' here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it classified as a method of matrix, how about that?

Copy link
Member

Choose a reason for hiding this comment

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

Okay, let's keep it as it is.

point->y = x * matrix->m[0][1] + y * matrix->m[1][1] + matrix->m[1][2];
}

void lv_matrix_transform_path(const lv_matrix_t * matrix, lv_vector_path_t * path)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
void lv_matrix_transform_path(const lv_matrix_t * matrix, lv_vector_path_t * path)
void lv_path_transform_by_matrix(lv_vector_path_t * path, const lv_matrix_t * matrix)

Signed-off-by: zhangjipeng <zhangjipeng@xiaomi.com>
@onecoolx onecoolx requested a review from XuNeo January 19, 2024 13:07
@C47D
Copy link
Contributor

C47D commented Jan 19, 2024

Will we add tests for this?

@kisvegabor
Copy link
Member

Will we add tests for this?

Some tests would be really great!

Signed-off-by: zhangjipeng <zhangjipeng@xiaomi.com>
Copy link
Contributor Author

@onecoolx onecoolx left a comment

Choose a reason for hiding this comment

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

add test cases

@kisvegabor
Copy link
Member

Thanks!

@kisvegabor kisvegabor merged commit 377a999 into lvgl:master Jan 23, 2024
16 checks passed
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

5 participants