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

Keyframe-based animation support #191

Merged
merged 32 commits into from Aug 4, 2018

Conversation

2 participants
@mosra
Owner

mosra commented Jan 22, 2017

Result of the weekend hackaton. Fun! Now revived after one and half a year. TODO:

  • Basic keyframe picking + interpolation algorithm
  • Track and TrackView APIs
    • duration() returning calculated duration of a clip, returning Range1D, suggesting Range::join() for combining (what to do with no keyframes?)
  • Correct interpolation of booleans
  • at least basic Hemite Spline interpolation, needed to verify design for complex interpolators postponed, design has been verified through a WIP implementation
  • A Clip class that handles playing, pausing etc. will be part of Player API
  • An Animator A Player class that handles playing multiple tracks, play/pause/stop, repeating
    • what about multiple clips? nope, use multiple players
  • Trade::AnimationData
  • Trade::ObjectData with separate TRS instead of a matrix

What's postponed to later has been moved to #101.

@mosra mosra added the feature label Jan 22, 2017

@mosra mosra referenced this pull request Jan 22, 2017

Open

Complete animation support #101

17 of 44 tasks complete
@codecov-io

This comment has been minimized.

codecov-io commented Jul 5, 2018

Codecov Report

Merging #191 into master will increase coverage by 1.06%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #191      +/-   ##
==========================================
+ Coverage   49.69%   50.76%   +1.06%     
==========================================
  Files         357      365       +8     
  Lines       16080    16431     +351     
==========================================
+ Hits         7991     8341     +350     
- Misses       8089     8090       +1
Impacted Files Coverage Δ
src/Magnum/Shaders/VertexColor.h 50% <ø> (ø) ⬆️
src/Magnum/Shaders/AbstractVector.h 40% <ø> (ø) ⬆️
src/Magnum/Trade/Trade.h 100% <ø> (ø) ⬆️
src/Magnum/GL/Attribute.h 69.23% <ø> (ø) ⬆️
src/Magnum/Trade/AbstractImporter.h 100% <ø> (ø) ⬆️
src/Magnum/Shaders/Flat.h 30.76% <ø> (ø) ⬆️
src/Magnum/Shaders/Phong.h 42.3% <ø> (ø) ⬆️
src/Magnum/Math/Range.h 95.53% <ø> (ø) ⬆️
src/Magnum/Shaders/MeshVisualizer.h 38.09% <ø> (ø) ⬆️
src/Magnum/GL/AbstractShaderProgram.h 5.26% <ø> (ø) ⬆️
... and 24 more

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 ae2281d...ea47a6e. Read the comment docs.

mosra added some commits Jul 4, 2018

Animation: make interpolation result type non-implicit.
TrackView can, for example, have quaternions encoded as a 10-10-10-2
integer, but the result should still be a Quaternion.
Animation: added Interpolation enum and interpolatorFor() helper.
Will be used to supply general desired interpolation method to tracks to
make it possible for the user to decide about a particular interpolator
function for given type.
Animation: make it possible to specify Track interpolator via an enum.
Or via an enum + func ptr. Main goal of this is to provide a hint to
users who want to supply their own interpolator (for example with a
different perf/correctness tradeoff, or a optimized/inlined/batch
version etc.).
Doxygen. Doxygen IS BROKEN BEYOND REPAIR.
I introduced *strong* enum with values, in a subnamespace, that have the
same name as completely unrelated typedefs. Guess what?! It breaks ALL
LINKS TO THOSE TYPEDEFS! **EVERYWHERE!!!**
Trade: convenience default constructor for AnimationTrackData.
Otherwise one would need to use a NoInit Array constructor and that
would cause dangling deleter function pointer call after the plugin gets
unloaded.
Animation: TrackViewStorage doesn't need to be *that* type-erased.
Moreover, this will prevent from passing e.g. integer-based keys to
Trade::AnimationData. And this also now allows me to add duration() to
Trade::AnimationData. I also moved all accessors that don't need a
concrete value type to this base class.
Trade: provide untyped access to tracks in AnimationData.
This reduces the templated code a bit, as I moved the index assertion to
the *.cpp file. Also the function now returns a reference to avoid
needless copies -- it's a view, but still quite a heavy view.
Animation: make TrackView constructors implicit.
For consistency with all other views such as Containers::ArrayView as
these are all relatively light types and so the construction should be
lightweight as well. OTOH, Track constructors are still explicit because
they're heavy and the user should experience the heaviness firsthand.
Animation: benchmark TrackView to verify my performance assumptions.
Turns out my performance assumptions were correct! :)
Trade: make it possible to use all vector type variations for animati…
…ons.

Note that I don't mean all possible underlying types, just all possible
vector classes. This is needed for splines etc. which may be templated
and have result type a generic vector, for example.

mosra added some commits Aug 2, 2018

Animation: added Player::addRawCallback().
Provides some further optimization opportunities.
Animation: Player move is not noexcept because compilers are weird to…
…day.

I managed to work around that for Emscripten 1.38.5 by adding an
explicit definition of noexcept Player::Track copy
constructor/assignment, but that didn't solve anything for Emscripten,
iOS or Android. Since I can't reproduce that on *anything* I have on
this machine and it seems that the problem somehow just goes away when
using a more recent Clang, I decided to just remove the noexcept
specifier altogether.

I had a temptation to make it noexcept everywhere except Clang, but that
might add potential portability issues (code that works with GCC would
suddenly break on Clang without any clear reason).
Animation: put a reminder for myself.
At the moment, with the major use case, which is playing back glTF
animations, one can't use atStrict() because there Extrapolation is
specified to be always Constant. The atStrict() function behaves as
Extrapolate and one would need to patch the imported tracks to add
explicit keyframes at the beginning/end of the duration that emulate the
Constant behavior.
Animation: make Player usable on MSVC.
MSVC 2015 and 2017 is clueless when it comes to trying to deduce the
template parameters (C2893: Failed to specialize function template). It
works when calling add<V, R> explicitly, but that makes the API hard to
use and inconsistent between platforms. The only possible workaround is
to make add() take *anything*, casting it to proper TrackView type and
then calling add() with explicit template parameters. This also neatly
resolves the Track/TrackView overload, as the static_cast is either a
no-op or it invokes the conversion operator on Track. The original code
also reportedly makes the Intellisense freezing like hell and adding
this overload fixes the freezes. Three birds with one stone.

The Player class definition is now full of typedefs, with the amount of
comments about why the typedefs are there much bigger than the actual
code. Oh well.
Animation: properly handle Player with empty duration.
Turns out with current design we can allow fun things even for empty
durations. I like this.
Animation: work around an ICE involving std::chrono on MSVC 2017.
I lost two hours on pinpointing this one. Gah.
doc: disable a problematic code snippet on WinRT.
It has warnings-as-errors and thus doesn't like that one uninitialized
object. I need the ELLIPSIS macro already.

@mosra mosra merged commit ea47a6e into master Aug 4, 2018

2 of 4 checks passed

continuous-integration/appveyor/branch Waiting for AppVeyor build to complete
Details
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

Math and algorithms automation moved this from In progress to Done Aug 4, 2018

@mosra

This comment has been minimized.

Owner

mosra commented Aug 4, 2018

Merged into master now, finally 🎉

@mosra mosra deleted the animation branch Aug 4, 2018

@mosra mosra changed the title from [WIP] Keyframe-based animation support to Keyframe-based animation support Aug 4, 2018

This was referenced Aug 4, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment