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

Add merge! for animation types, insert! for AnimationTrack. #115

Merged
merged 11 commits into from
Aug 5, 2019

Conversation

tkoolen
Copy link
Collaborator

@tkoolen tkoolen commented Jul 10, 2019

On top of #112.

Needs tests, but seems to be working fine for me. Just wanted to open this for review and so that I don't forget.

@codecov-io
Copy link

codecov-io commented Jul 11, 2019

Codecov Report

Merging #115 into master will decrease coverage by 0.54%.
The diff coverage is 69.23%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #115      +/-   ##
=========================================
- Coverage   76.04%   75.5%   -0.55%     
=========================================
  Files          14      15       +1     
  Lines         405     449      +44     
=========================================
+ Hits          308     339      +31     
- Misses         97     110      +13
Impacted Files Coverage Δ
src/lowering.jl 83.09% <ø> (ø) ⬆️
src/MeshCat.jl 30.43% <ø> (ø) ⬆️
src/atframe.jl 83.87% <100%> (+6.72%) ⬆️
src/util.jl 100% <100%> (ø)
src/animations.jl 57.14% <42.85%> (-19.05%) ⬇️

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 d1bec32...ca6e588. Read the comment docs.

src/animations.jl Outdated Show resolved Hide resolved
src/animations.jl Outdated Show resolved Hide resolved
@rdeits
Copy link
Owner

rdeits commented Jul 12, 2019

This seems totally reasonable. Let's do it!

@tkoolen tkoolen force-pushed the tk/merge-animations branch from f8e847d to 3ae5048 Compare July 12, 2019 11:48
function Base.merge!(a::AnimationClip, others::AnimationClip...)
for other in others
@assert other.fps == a.fps
merge!(merge!, a.tracks, other.tracks) # merge tracks recursively
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was just thinking: there could potentially be two animation tracks with the same name but different jstypes. That would currently error here. Again, not with the current atframe implementation,

track = get!(clip.tracks, prop) do
AnimationTrack(prop, jstype, Int[], wider_js_type(typeof(value))[])
end

but still.

@tkoolen tkoolen force-pushed the tk/merge-animations branch from 3078385 to fd6cd97 Compare July 18, 2019 17:45
@rdeits rdeits mentioned this pull request Jul 28, 2019
@tkoolen tkoolen force-pushed the tk/merge-animations branch from fd6cd97 to 888c2bc Compare August 5, 2019 01:30
@tkoolen tkoolen force-pushed the tk/merge-animations branch from 3505532 to ca6e588 Compare August 5, 2019 02:40
@rdeits rdeits merged commit 0dba4b1 into rdeits:master Aug 5, 2019
@rdeits
Copy link
Owner

rdeits commented Aug 5, 2019

Thanks!

@tkoolen tkoolen deleted the tk/merge-animations branch August 5, 2019 03:52
@tkoolen
Copy link
Collaborator Author

tkoolen commented Aug 5, 2019

Actually, there's still an issue with merging AnimationTracks. I'm working on a test + fix. I'll open another PR.

@rdeits
Copy link
Owner

rdeits commented Aug 5, 2019

Ok, no problem

@tkoolen
Copy link
Collaborator Author

tkoolen commented Aug 5, 2019

#124

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.

3 participants