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

Use Cassette to record animations #112

Merged
merged 4 commits into from
Jul 12, 2019

Conversation

tkoolen
Copy link
Collaborator

@tkoolen tkoolen commented Jul 9, 2019

I realized today that Cassette is exactly the right tool to record visualizations. Even the name fits.

I've disliked AnimationFrameVisualizer for a while; it's almost a Visualizer but not quite, and it makes it harder to set up animations for specific purposes. For example, in MeshCatMechanisms we have a setanimation! overload with the lines:

        atframe(animation, visualizer(mvis), frame) do frame_visualizer
            _render_state!(MechanismVisualizer(state(mvis), frame_visualizer))
        end

Even though we already had a MechanismVisualizer all set up and ready to go, we're creating a new one at every frame from frame_visualizer, just so we can use the same API to modify the underlying AnimationFrameVisualizer. This makes MechanismVisualizer more complicated than it needs to be (parameterized on the type of its visualizer field), and it means that you can't really do any useful work in the MechanismVisualizer constructor.

The basic goal of AnimationFrameVisualizer is to intercept calls to setprop! and settransform!, and Cassette is designed for just that. With Cassette, there's no longer a need to parameterize MechanismVisualizer (or any MeshCat.Visualizer-holder) on the visualizer type in order to support animation, and

atframe(anim, vis, 30) do frame
    settransform!(frame[:box1], Translation(0., 1, 0))
end

simplifies to

atframe(anim, 30) do
    settransform!(vis[:box1], Translation(0., 1, 0))
end

I kept the old AnimationFrameVisualizer for now, but I actually think we can just get rid of it entirely in a pretty-much backwards-compatible fashion by just returning vis as frame_vis in the four-argument version of atframe.

@codecov-io
Copy link

codecov-io commented Jul 9, 2019

Codecov Report

Merging #112 into master will decrease coverage by 1.47%.
The diff coverage is 77.14%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #112      +/-   ##
=========================================
- Coverage   77.58%   76.1%   -1.48%     
=========================================
  Files          14      14              
  Lines         397     406       +9     
=========================================
+ Hits          308     309       +1     
- Misses         89      97       +8
Impacted Files Coverage Δ
src/MeshCat.jl 30.43% <ø> (ø) ⬆️
src/animations.jl 76.19% <ø> (ø) ⬆️
src/atframe.jl 77.14% <77.14%> (ø)
src/visualizer.jl 69.73% <0%> (-5.98%) ⬇️

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 66a3c1d...c36f197. Read the comment docs.

@rdeits
Copy link
Owner

rdeits commented Jul 10, 2019

Wow, this is a really good idea! I had never thought of handling animation this way, but it makes a lot of sense. If this works well for you I'm happy to delete the old AnimationVisualizer.

I think this should also make animating arrows easier, since they previously had the same problem that MechanismVisualizer did, where you had to recreate the ArrowVisualizer inside each frame of the animation.

@tkoolen
Copy link
Collaborator Author

tkoolen commented Jul 10, 2019

Added the deprecation and removed AnimationFrameVisualizer. For the atframe method that takes a Path we unfortunately have to throw an error.

@tkoolen
Copy link
Collaborator Author

tkoolen commented Jul 11, 2019

I've been using this for a couple of days now and haven't run into any issues (after working around and later fixing JuliaLabs/Cassette.jl#89). So if you're also happy with this change, I'd love to get it merged.

@rdeits
Copy link
Owner

rdeits commented Jul 12, 2019

Yeah, this is great. Merging!

@rdeits rdeits merged commit 160933c into rdeits:master Jul 12, 2019
@tkoolen tkoolen deleted the tk/cassette-animation branch July 12, 2019 03:18
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