-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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
[Draft] Animation API (#5350) #6197
base: master
Are you sure you want to change the base?
Conversation
f27c47d
to
93ba545
Compare
src/misc/lv_anim.c
Outdated
/*Creating an animation changed the linked list. | ||
*It's important if it happens in a ready callback. (see `anim_timer`)*/ | ||
anim_mark_list_change(); | ||
lv_anim_t * lv_anim_clone(const lv_anim_t * other) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have a word dup
used in draw buffer. How about lv_anim_dup
? Please go ahead if clone is more suitable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think dup
is not the best keyword. copy
or clone
could be better. So we should rename lv_draw_buf_dup
too to lv_draw_buf_copy/clone
and add a define in lv_api_map_v9.h
for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've renamed lv_draw_buf_dup
. Please check if the api map is correct
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Turns out dup
is kind of a standard abbreviation in C (strdup) and POSIX.
I'd go with lv_anim_dup
and addig it to CODE_STYLE.rst
.
@kisvegabor
return new_anim; | ||
} | ||
|
||
void lv_anim_trigger(lv_anim_t * a) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we support trigger same animation again and again?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's supported by:
// configure once
lv_anim_t* a_reusable = lv_anim_create();
// lv_anim_set_*(a_reusable, ...
// call again and again
lv_anim_trigger(lv_anim_clone(a_reusable));
A @kisvegabor mentioned below, we are having trouble fitting the "user managed memory" paradigm with a consistent API. I can imagine 2 main scenarios:
-
Keep the current - stack allocated, then internally copied - API and extend it with "advanced" features, like:
- add a
tag
field to each animation, then calllv_anim_delete_by_tag()
to free a subset of animations - "smart pointers" returned by "lv_anim_start()", which indicate if the underlying memory has been freed in the meantime
Also we might need to change our mindset. The fact that
anim
does not havecreate
method is not a inconsistency in the API, it's a notification for users that thecreate
/delete
pattern does not apply here due to the internal mechanism - add a
-
Introduce the new dynamic allocation API (as seen in the PR). The issue here is to keep the default - and in most cases very useful - behavior of freeing upon completion. (Another issue is
anim_timeline
module, which creates internal copies too, but never triggers / starts them). Maybe changing tolv_anim_tigger_single()
with freeing upon completionlv_anim_tigger_reusable()
without freeing upon completion
would make sense
src/misc/lv_anim.c
Outdated
return a->is_running; | ||
} | ||
|
||
void lv_anim_set_act_time(lv_anim_t * a, uint32_t act_time) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
void lv_anim_set_act_time(lv_anim_t * a, uint32_t act_time) | |
void lv_anim_set_duration(lv_anim_t * a, uint32_t duration) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think act_time
is correct here as this function seeks in the animation. However, act_time
itself is a wrong a name. It's coming from my bad English from many years ago. 😄
As anim->act_time
is used only internally we can safely rename it to current_time
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
renamed
@@ -54,27 +54,50 @@ void _lv_ll_init(lv_ll_t * ll_p, uint32_t node_size) | |||
ll_p->n_size = node_size; | |||
} | |||
|
|||
void * _lv_ll_ins_head(lv_ll_t * ll_p) | |||
void * _lv_ll_node_allocate(lv_ll_t * ll_p) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think lvgl intends to make the alloc inside insert
operation. Let's keep it or rewrite it following https://github.com/torvalds/linux/blob/master/include/linux/list.h(which is implemented also in many rtos(https://github.com/apache/nuttx/blob/master/include/nuttx/list.h))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's shown oddly in the diff, but in lv_ll.c
we still have _lv_ll_ins_head
and _lv_ll_node_allocate
is added just as an option to allocate a "floating" node.
@@ -133,6 +139,8 @@ struct _lv_anim_t { | |||
lv_anim_custom_exec_cb_t custom_exec_cb;/**< Function to execute to animate, | |||
same purpose as exec_cb but different parameters*/ | |||
lv_anim_start_cb_t start_cb; /**< Call it when the animation is starts (considering `delay`)*/ | |||
lv_anim_pause_cb_t pause_cb; /**< Call it when the animation is paused */ | |||
lv_anim_resume_cb_t resume_cb; /**< Call it when the animation is resumed */ | |||
lv_anim_completed_cb_t completed_cb; /**< Call it when the animation is fully completed*/ | |||
lv_anim_deleted_cb_t deleted_cb; /**< Call it when the animation is deleted*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the memory is allocated by user now, let's avoid free the memory users allocates.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In most of the cases the users want the animation to be deleted automatically on competition. However I do understand for better life cycle management you might need the animation pointers. Can you mention a use case when the animation is not deleted automatically with the widgets they are animating, but you are managing the animation separately?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a use case when the animation is not deleted automatically
What about a animation when users clicks screen over and over? Reuse the previous animation already setup sounds reasonable.
The complete life cycle management actually comes from the binding language, where the memory should be free'd at certain time. For example, lua
code may still hold a reference to anim
handler while it's already automatically deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reusing the animation is possible as of the PR, but see my comment making it more streamlined above.
I don't clearly see the relevance of the problems raised by the binding language.
- Freeing at certain time is safe and possible by
lv_anim_delete()
(Right nowlv_anim_delete_by_ptr()
can lead to double freeing if the animation has finished beforehand,lv_anim_tigger_reusable()
would solve that.) - Is "holding a reference to deleted handlers" truly animation specific? I'd imagine the same thing would happen with a widget that is being closed, or any other lvgl object, that is being freed by internal mechanisms, like patent-child "garbage collection".
I'm not so familiar with lua. Would you mind creating an example script where this issue is demonstrated? It would be a big help.
|
||
resolve_time(a); | ||
|
||
/*Do not let two animations for the same 'var' with the same 'exec_cb'*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please, why we can't have two animations with same var to same exec callback?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would lead to concurrency (who got registered to the list first) and undefined behavior.
Imagine 2 animations setting the y offset of the same ball with a time delay between them. Which one is right? Where the ball should really be?
0854a25
to
1064431
Compare
1064431
to
f4002b7
Compare
We need some feedback on this pull request. Now we mark this as "stale" because there was no activity here for 14 days. Remove the "stale" label or comment else this will be closed in 7 days. |
Not stale |
This PR implements #5350