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

Removing quadrilateral interpolation #5740

Merged
merged 7 commits into from
Jan 10, 2018
Merged

Conversation

moneimne
Copy link
Contributor

There were jitter issues with the Polly model because of the quadrilateral interpolation. Spoke to @lilleyse and he said to remove it as long as it was causing issues. I tested Polly and other models with these changes and things seem to work fine.

@pjcozzi pjcozzi requested a review from lilleyse August 19, 2017 17:57
@mramato
Copy link
Contributor

mramato commented Sep 27, 2017

@lilleyse is this PR still valid?

@lilleyse
Copy link
Contributor

Yeah, I've been meaning to look at this. The code seems fine I just need to test a bunch of models.

@lilleyse
Copy link
Contributor

lilleyse commented Jan 9, 2018

My review is long overdue here. A model with some animation issues came up on the forum and was fixed by using this branch. I also tested all of the animated glTFs in https://github.com/KhronosGroup/glTF-Sample-Models/tree/master/2.0 which work fine on this branch.

The code looks good. I just made a doc fix and added a note to CHANGES.md.

Overall slerp seems to be more stable than squad, and I can't see a difference in animation quality.

@emackey - I'm curious if you have a take on this.

@emackey
Copy link
Contributor

emackey commented Jan 9, 2018

One deficiency in our current set of sample models is that all of our animated models use the default LINEAR interpolation. Technically the glTF model is able to specify a type of interpolation as LINEAR, STEP, or CUBICSPLINE, but we don't have samples of the other two yet.

Even so, if this makes things noticeably better (I didn't try it myself yet), then I think it's fine to merge this now and revisit in more detail when more rigorous tests become available.

@lilleyse
Copy link
Contributor

Thanks @emackey.

@lilleyse lilleyse merged commit 271d6b3 into CesiumGS:master Jan 10, 2018
@cesium-concierge
Copy link

Congratulations on closing the issue! I found these Cesium forum links in the comments above:

https://groups.google.com/forum/#!topic/cesium-dev/Ktc9vwJiiTU

If this issue affects any of these threads, please post a comment like the following:

The issue at #5740 has just been closed and may resolve your issue. Look for the change in the next stable release of Cesium or get it now in the master branch on GitHub https://github.com/AnalyticalGraphicsInc/cesium.


I am a bot who helps you make Cesium awesome! Contributions to my configuration are welcome.

🌍 🌎 🌏

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.

5 participants