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

Lottie: Animation Segment(Marker) Support #2080

Closed
wants to merge 3 commits into from

Conversation

fall2019
Copy link
Contributor

@fall2019 fall2019 commented Mar 21, 2024

issue: #2044

@fall2019 fall2019 requested a review from hermet March 21, 2024 11:54
@hermet hermet added lottie Lottie animation feature New feature additions labels Mar 21, 2024
@fall2019 fall2019 marked this pull request as ready for review March 21, 2024 18:11
@fall2019 fall2019 changed the title Lottie Markers Lottie: Animation Segment(Marker) Support Mar 22, 2024
@hermet hermet requested a review from tinyjin March 22, 2024 07:12
@hermet
Copy link
Member

hermet commented Mar 22, 2024

@fall2019, could you please squash your unnecessary commits to the one?

@fall2019
Copy link
Contributor Author

Okay. I usually use squash on merge option. Will try to squash later.

@fall2019
Copy link
Contributor Author

Squashed all commits into one.

inc/thorvg.h Outdated Show resolved Hide resolved
Copy link
Member

@tinyjin tinyjin left a comment

Choose a reason for hiding this comment

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

Hello, Thank you so much for the efforts.
We'd love to have your patches.

Please check these comments:

1. Conflict Resolution

  • src/loaders/lottie/tvgLottieLoader.h currently has a merge conflict.
  • Please ensure that the patch is conflict-free before merging.

2. Enhance Commit Message Clarity

  • The current message simply states "POC"
  • It would be beneficial to provide a more descriptive message that outlines the purpose of these changes.
  • We strongly recommend referring to this guidance for composing meaningful commit messages.

3. Improve Commit Description Formatting:

  • The commit description appears unclear.
  • It seems to be just squashed without tidying the explanations, there are a lot of typos or unnecessary texts.
  • Please check again the description is clear and concise.
    Screenshot 2024-03-22 at 7 32 49 PM

@fall2019
Copy link
Contributor Author

Okay. Thanks for reviewing and your article is really helpful. Will resolve the comments later.

Copy link
Member

@hermet hermet left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution, quickly examined the API design.

inc/thorvg.h Outdated Show resolved Hide resolved
inc/thorvg.h Outdated Show resolved Hide resolved
inc/thorvg.h Outdated Show resolved Hide resolved
@fall2019
Copy link
Contributor Author

You're welcome. Will try to resolve soon.

@fall2019 fall2019 force-pushed the lucas/markers branch 2 times, most recently from bcaeab9 to 60c58d0 Compare March 23, 2024 05:49
@fall2019
Copy link
Contributor Author

Resolved all comments.

@fall2019 fall2019 requested a review from tinyjin March 23, 2024 06:40
@fall2019
Copy link
Contributor Author

fall2019 commented Apr 3, 2024

Alright. Resolved.

Copy link
Member

@hermet hermet left a comment

Choose a reason for hiding this comment

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

Reviewed "Animation Segment(Marker) Support" commit. It looks almost there. Thanks,

src/loaders/lottie/tvgLottieModel.h Outdated Show resolved Hide resolved
src/loaders/lottie/tvgLottieLoader.cpp Show resolved Hide resolved
examples/resources/lottie/marker_sample.json Outdated Show resolved Hide resolved
src/loaders/lottie/thorvg_lottie.h Show resolved Hide resolved
src/loaders/lottie/thorvg_lottie.h Outdated Show resolved Hide resolved
src/loaders/lottie/thorvg_lottie.h Show resolved Hide resolved
src/loaders/lottie/tvgLottieParser.cpp Outdated Show resolved Hide resolved
src/loaders/lottie/tvgLottieModel.h Outdated Show resolved Hide resolved
@hermet
Copy link
Member

hermet commented Apr 4, 2024

@fall2019 One more, please add yourself "https://github.com/thorvg/thorvg/blob/main/AUTHORS" please.

@fall2019
Copy link
Contributor Author

fall2019 commented Apr 4, 2024

Thanks. Resolved all comments.

@fall2019 fall2019 requested a review from hermet April 4, 2024 06:38
Copy link
Member

@tinyjin tinyjin left a comment

Choose a reason for hiding this comment

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

Please check comments.

examples/LottieExtension.cpp Outdated Show resolved Hide resolved
src/loaders/lottie/tvgLottieAnimation.cpp Outdated Show resolved Hide resolved
src/loaders/lottie/thorvg_lottie.h Outdated Show resolved Hide resolved
src/loaders/lottie/tvgLottieLoader.cpp Show resolved Hide resolved
src/loaders/lottie/tvgLottieAnimation.cpp Outdated Show resolved Hide resolved
examples/LottieExtension.cpp Outdated Show resolved Hide resolved
src/loaders/lottie/thorvg_lottie.h Show resolved Hide resolved
src/loaders/lottie/thorvg_lottie.h Show resolved Hide resolved
@tinyjin
Copy link
Member

tinyjin commented Apr 5, 2024

@hermet
I've tested segment API in various suites, seems nicely working. (some minor consideration -> commented)

Here, I wanna make sure two things.

1. Unit test
Should we include unit test for marker & segment in this patch? Otherwise, I can also add all cases I've checked into testLottie.cpp and capiLottie.cpp right after merging.

2. Support marker comment in JSON objects.

"markers": [
    { "tm": 0, "cm": "{\\"name\\":\\"bird\\"}", "dr": 22 },
    { "tm": 22, "cm": "{\\"name\\":\\"explosion\\"}", "dr": 11 },
    { "tm": 33, "cm": "{\\"name\\":\\"feather\\"}", "dr": 77 }
]

This is also from RFC, the markers[].cm could be JSON string. Do you think it must be in this patch? I think we can gradually support this case in next patch.

@hermet
Copy link
Member

hermet commented Apr 5, 2024

1. Unit test
Should we include unit test for marker & segment in this patch? Otherwise, I can also add all cases I've checked into testLottie.cpp and capiLottie.cpp right after merging.

@tinyjin Yes, It would be better to perform unit testing, but it's not mandatory in this PR.

@fall2019 Since on this topic, Returning the list of marker names will be essential, so users can inquire about the available markers in the animation. Probably, like this:

uint32_t LottieAnimation::markerCnt();  //return the marker count
const char* LottieAnimation::markers(uint32_t idx); //return the marker name by a given index.

The items(unit test and apis) mentioned above are not the main focus at this point but will become essential eventually. Therefore, it's up to you. If you'd like to include them, please update the PR accordingly. Thanks.



2. Support marker comment in JSON objects.

"markers": [
    { "tm": 0, "cm": "{\\"name\\":\\"bird\\"}", "dr": 22 },
    { "tm": 22, "cm": "{\\"name\\":\\"explosion\\"}", "dr": 11 },
    { "tm": 33, "cm": "{\\"name\\":\\"feather\\"}", "dr": 77 }
]

This is also from RFC, the markers[].cm could be JSON string. Do you think it must be in this patch? I think we can gradually support this case in next patch.

@tinyjin No, it shouldn't be.

Copy link
Member

@hermet hermet left a comment

Choose a reason for hiding this comment

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

Reviewed "Add example for Animation Segment(Marker)"

examples/LottieExtension.cpp Outdated Show resolved Hide resolved
examples/LottieExtension.cpp Show resolved Hide resolved
@fall2019
Copy link
Contributor Author

fall2019 commented Apr 9, 2024

Add api
uint32_t LottieAnimation::markerCnt(); //return the marker count const char* LottieAnimation::markers(uint32_t idx); //return the marker name by a given index.

Resolved all comments.

@fall2019 fall2019 requested review from tinyjin and hermet April 9, 2024 11:54
A single animation might have a desinated markers with naming: 0 ~ 0.5 (sector A), 0.5 ~ 1.0  (sector B). Selecting one of them using a marker name(sector A) and could play only that part with animation controllers.
@tinyjin
Copy link
Member

tinyjin commented Apr 11, 2024

@fall2019. So far pretty good.
It's around the corner. I'm going to soon wrap up this patch by my side.

Thanks.

@fall2019
Copy link
Contributor Author

Okay. On your side.

@tinyjin
Copy link
Member

tinyjin commented Apr 11, 2024

#2164

@tinyjin tinyjin closed this Apr 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature additions lottie Lottie animation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants