✨ Add LKAnimationKit #217

YannLocatelli · 2021-04-12T12:57:28Z

No description provided.

codecov · 2021-04-12T13:03:20Z

Codecov Report

Merging #217 (9ab5bbe) into develop (6c6323a) will not change coverage.
The diff coverage is 100.00%.

❗ Current head 9ab5bbe differs from pull request most recent head 7ed30a5. Consider uploading reports for the commit 7ed30a5 to get more accurate results

@@            Coverage Diff            @@
##           develop      #217   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           40        42    +2     
  Lines          651       665   +14     
=========================================
+ Hits           651       665   +14

Impacted Files	Coverage Δ
libs/LKAnimationKit/include/internal/CGAnimation.h	`100.00% <100.00%> (ø)`
libs/LKAnimationKit/source/LKAnimationKit.cpp	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6c6323a...7ed30a5. Read the comment docs.

ladislas

premier passage! :)

libs/CMakeLists.txt

libs/LKAnimationKit/CMakeLists.txt

libs/LKAnimationKit/include/LKAnimationKit.h

libs/LKAnimationKit/source/LKAnimationKit.cpp

spikes/lk_cg_animations/CMakeLists.txt

libs/LKAnimationKit/source/LKAnimationKit.cpp

libs/LKAnimationKit/include/LKAnimationKit.h

tests/unit/mbed-os/stubs/stub_EventQueue_extended.cpp

ladislas

on est quasi bon! quelques petits points de forme et un ajout sur les tests et on est bon 👍

libs/CMakeLists.txt

libs/LKAnimationKit/include/internal/CGAnimation.h

ladislas · 2021-04-19T10:52:30Z

libs/LKAnimationKit/source/LKAnimationKit.cpp

+	_animation.stop();
+}
+
+void LKAnimationKit::setRefreshRate(std::chrono::milliseconds rate)


quelles peuvent être les raisons de changer le refresh rate?

quel est l'impact? ne doit-on pas le passer aussi à CGAnimation pour pas qu'elle puisse s'adapter (on ne veut pas que le cube aille trop vite ou trop lentement)

Il n'y a pas de notion de temps dans les CGAnimation

Celui qui gère le temps, que ce soit avec des ThisThread::sleep_for() ou lors du event_queue, c'est dans LKAnimationKit. C'est l'appel de run() dans LKAnimationKit qui conditionnera la vitesse de l'animation

La raison pour le changer sont

D'un point de vue esthétique, chaque animation à des vitesse différentes

Dans LKAnimationKit, garder le contrôle du "Temps" à haut niveau

Il n'y a pas de notion de temps dans les CGAnimation

c'est pas l'object de la PR mais en fait si c'est là que ça doit être

D'un point de vue esthétique, chaque animation à des vitesse différentes

mais ça LKAnimationKit ne le sait pas, faut donc que l'utilisateur se rappelle de mettre le "bon temps"

Dans LKAnimationKit, garder le contrôle du "Temps" à haut niveau

je ne comprends pas ça

C'est l'appel de run() dans LKAnimationKit qui conditionnera la vitesse de l'animation

en animation en général tu veux justement tout l'inverse : si ton ordi rame sur une tâche lourde et que tu joues une vidéo en même temps, le durée absolue de la vidéo ne va pas changer, la vidéo ne sera pas jouée au ralenti. par contre tu vas avoir du lag avec des moments de freeze puis une courte séquence, freeze, et ainsi de suite. c'est le CPU qui donne trop peu de temps au process de la vidéo pour avancer correctement.

c'est au lecteur de s'adapter

ici c'est pareil:

une animation elle doit avoir une version de base avec une durée définie et une fréquence optimale définies aussi

tu peux vouloir la jouer plus ou moins vite mais je ne vois pas de cas d'usage pour ça

connaissant sa durée, tu sais dire à chaque fois que tu appelles run() ou tu dois en être. tu peux donc corriger si ça rame pour ne pas avoir l'animation qui dure plus longtemps que prévu e et qui risque donc de décaler tout le reste dans le temps

bien sûr le lag n'est pas ce qu'on recherche, mais c'est le moindre mal vis à vis du timing.

cela dit, ce n'est pas l'objet de la PR, on pourra revoir cela plus tard.

libs/LKAnimationKit/tests/LKAnimationKit_test.cpp

tests/unit/mbed-os/mocks/mock_CGAnimation.h

tests/unit/mbed-os/stubs/stub_EventQueue_extended.cpp

ladislas

encore trois trucs et on merge 👍

libs/LKAnimationKit/include/LKAnimationKit.h

libs/LKAnimationKit/include/internal/CGAnimation.h

libs/LKAnimationKit/source/LKAnimationKit.cpp

ladislas

3 points de formes et une question sur le setRefreshDuration

libs/LKAnimationKit/tests/LKAnimationKit_test.cpp

libs/LKAnimationKit/source/LKAnimationKit.cpp

ladislas · 2021-04-20T09:30:17Z

libs/LKAnimationKit/tests/LKAnimationKit_test.cpp

+	auto refresh_duration = std::chrono::milliseconds(42);
+
+	animationkit.setRefreshDuration(refresh_duration);


ici on test pas que dispatch est appelé avec le bon refresh duration? (voir comment plus haut)

Il faut faire un mock de event_queue, c'était pas spécialement prévu mais si c'est indispensable je peux le faire

spikes/lk_cg_animations/CMakeLists.txt

codeclimate · 2021-04-20T12:08:34Z

Code Climate has analyzed commit 7ed30a5 and detected 0 issues on this pull request.

View more on Code Climate.

sonarcloud · 2021-04-20T12:15:02Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
26 Code Smells

100.0% Coverage
7.4% Duplication

YannLocatelli requested a review from ladislas April 12, 2021 12:57

YannLocatelli mentioned this pull request Apr 12, 2021

✨ Add LKCoreAnimation (obsolete) #200

Closed

8 tasks

This was referenced Apr 12, 2021

✨ Add Animation BouncingSquare #218

Merged

✨ Add Animation UnrealRainbow #219

Closed

YannLocatelli changed the title ~~Add LKAnimationKit~~ ✨ Add LKAnimationKit Apr 12, 2021

ladislas reviewed Apr 12, 2021

View reviewed changes

YannLocatelli force-pushed the yann/images_and_animations/add-LKAnimationKit branch 2 times, most recently from 4a70ce6 to 4537fd0 Compare April 13, 2021 13:40

ladislas reviewed Apr 13, 2021

View reviewed changes

YannLocatelli commented Apr 19, 2021

View reviewed changes

tests/unit/mbed-os/stubs/stub_EventQueue_extended.cpp Outdated Show resolved Hide resolved

YannLocatelli force-pushed the yann/images_and_animations/add-LKAnimationKit branch 2 times, most recently from fb32c83 to 6a5d68c Compare April 19, 2021 08:49

YannLocatelli requested a review from ladislas April 19, 2021 08:49

ladislas requested changes Apr 19, 2021

View reviewed changes

YannLocatelli force-pushed the yann/images_and_animations/add-LKAnimationKit branch 4 times, most recently from fead98e to 1e4347a Compare April 19, 2021 13:58

ladislas reviewed Apr 19, 2021

View reviewed changes

YannLocatelli force-pushed the yann/images_and_animations/add-LKAnimationKit branch 2 times, most recently from 9dcfce6 to d71a6d8 Compare April 20, 2021 09:05

ladislas reviewed Apr 20, 2021

View reviewed changes

YannLocatelli force-pushed the yann/images_and_animations/add-LKAnimationKit branch from 9ab5bbe to 167fa8a Compare April 20, 2021 10:53

YannLocatelli added 2 commits April 20, 2021 13:02

✨ (animations): Add CGAnimation

e7e14e8

✨ (animations): Add LKAnimationKit

7e5478f

YannLocatelli force-pushed the yann/images_and_animations/add-LKAnimationKit branch from 167fa8a to 5a2e531 Compare April 20, 2021 11:03

ladislas approved these changes Apr 20, 2021

View reviewed changes

YannLocatelli added 2 commits April 20, 2021 14:07

✅ (animations): Add LKAnimationKit tests

edbe453

✨ (animations): Add Animations spike

7ed30a5

ladislas force-pushed the yann/images_and_animations/add-LKAnimationKit branch from 5a2e531 to 7ed30a5 Compare April 20, 2021 12:08

ladislas merged commit f70dbab into develop Apr 20, 2021

ladislas deleted the yann/images_and_animations/add-LKAnimationKit branch April 20, 2021 13:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Add LKAnimationKit #217

✨ Add LKAnimationKit #217

YannLocatelli commented Apr 12, 2021

codecov bot commented Apr 12, 2021 •

edited

Loading

ladislas left a comment

ladislas left a comment

ladislas Apr 19, 2021

YannLocatelli Apr 19, 2021

ladislas Apr 19, 2021

ladislas left a comment

ladislas left a comment

ladislas Apr 20, 2021

YannLocatelli Apr 20, 2021

codeclimate bot commented Apr 20, 2021

sonarcloud bot commented Apr 20, 2021

		auto refresh_duration = std::chrono::milliseconds(42);

		animationkit.setRefreshDuration(refresh_duration);

✨ Add LKAnimationKit #217

✨ Add LKAnimationKit #217

Conversation

YannLocatelli commented Apr 12, 2021

codecov bot commented Apr 12, 2021 • edited Loading

Codecov Report

ladislas left a comment

Choose a reason for hiding this comment

ladislas left a comment

Choose a reason for hiding this comment

ladislas Apr 19, 2021

Choose a reason for hiding this comment

YannLocatelli Apr 19, 2021

Choose a reason for hiding this comment

ladislas Apr 19, 2021

Choose a reason for hiding this comment

ladislas left a comment

Choose a reason for hiding this comment

ladislas left a comment

Choose a reason for hiding this comment

ladislas Apr 20, 2021

Choose a reason for hiding this comment

YannLocatelli Apr 20, 2021

Choose a reason for hiding this comment

codeclimate bot commented Apr 20, 2021

sonarcloud bot commented Apr 20, 2021

codecov bot commented Apr 12, 2021 •

edited

Loading