Cubic Hermite splines #267
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
AbstractImporter Documentation subtitle lists what it is able to import and missing "animations" there :) (as requested to put here via gitter) |
The totally random places after were not exactly useful. This file was sitting here for a while, not sure why I didn't commit that earlier.
Not sure why that wasn't done before.
It's a straight copy of the code for quaternions -- it could probably be
simplified a bit, but I don't have the necessary brain cells at the
moment. I tried the following but failed:
retun Complex::rotation(acos(cosAngle)*t)*normalizedA;
Not sure why these were omitted. Everything else has them.
Since there's now lerp() and slerp() for it.
For spline interpolation.
|
While this is all green and happy, there are blockers in depending projects:
|
Codecov Report
@@ Coverage Diff @@
## master #267 +/- ##
==========================================
+ Coverage 50.92% 51.26% +0.34%
==========================================
Files 365 367 +2
Lines 16586 16703 +117
==========================================
+ Hits 8446 8563 +117
Misses 8140 8140
Continue to review full report at Codecov.
|
No need to have the precision so crazy.
And causes an assertion message. Obviously the test doesn't pass anymore after this fix.
The fix done with #122 (0e05c72) was not tested properly (see previous commit) and thus this code path never worked. This properly lerps the translation part and recombines it with the rotation instead of interpolating just a part of it. Also I'm no longer having any "dotResult" that's done only on the vector part of the rotation, but instead using the full rotation quaternion dot product. I have no idea why it was done this way. This branch was also never properly tested -- it'll be with the introduction of "shortest path" variants in the next commit.
…p(). Before neither of the lerp(), slerp() had the shortest path check, while sclerp() had it. Now, to be consistent, none of them has it and there are lerpShortestPath(), slerpShortestPath() and sclerpShortestPath() functions that have the shortest path check. This is different from other engines, where there's usually only the shortest path interpolation by default and either an optional "non-shortest-path" interpolation or no alternative at all. I like to give the users a choice, so there's both versions and the non-shortest-path version is the default, because -- at least in case of lerp() -- this results in a quite significant perf difference (15% faster), so why not have it. Preprocess your data instead ;)
The function has an explicit name but making it the default makes for a less surprising behavior.
Also remove a totally unneeded conversion.
|
Merged into master. Shortest path interpolation is part of this PR as well, the remaining issues are glTF-specific, so handling them in the glTF importer itself instead. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Important for animations, among other things, postponed from #191 to unblock progress, but still hopefully on track for 2018.08. Cubic Hermite splines were chosen because they are a superset of all other commonly used splines and thus having support for these will also mean support for Catmull-Rom, Bezier, TCB and others.
A "point" representation was chosen over a "segment" representation in order to better support the use in animations. Math::Bezier, OTOH, has a segment representation.
TODO:
Catmull-Rompostponed, see Complete animation support #101TCBpostponed, see Complete animation support #101The representation that glTF usesno idea how the heck is that called, so it'll be just private to the glTF importerBezier::fromCubicHermite()(just because I have the class)Math::select()Math::lerp()Math::splerp()Math::lerp()for quaternionsMath::splerp()for quaternionsAnimation::Interpolation::Splineand support in related APIsTrade::AnimationDataTypeAPIslerp(),slerp()andsclerp()interpolatorFor()