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 CZML velocity vector orientation support. #5807

Merged
merged 1 commit into from
Sep 27, 2017
Merged

Conversation

shunter
Copy link
Contributor

@shunter shunter commented Sep 1, 2017

  • Add support for orientation defined as the client-side calculation of the velocity direction, via the existing velocityReference concept. As before, this still implies finite differencing, with the same limitations as before. Fixes Ability to specify orientation with VelocityOrientationProperty AnalyticalGraphicsInc/czml-writer#140
  • Improve handling of existing alignedAxis velocity support. We now distinguish properties which must produce a unit cartesian (currently only alignedAxis). For those properties, we always normalize values on the client to avoid later errors.
  • Fix velocityReference use from within an interval. Previously we handled reference specially while processing. Generalized this concept to instead consider them "specialized" (as opposed to plain value) properties. This now includes the client-side velocity calculating properties, and potentially others in the future. As a result a lot of the logic was inverted throughout the processing. Fixes velocityReference is not supported within an interval. #5738
  • Fix promise handling in CzmlDataSourceSpec. Many tests were calling .load without using the resulting promise. This meant that any exceptions during CZML load would be silently ignored by the tests. Remember, any test code that calls a method returning a promise but doesn't use the result is probably wrong!

@cesium-concierge
Copy link

@shunter, thanks for the pull request! Maintainers, we have a signed CLA from @shunter, so you can review this at any time.

I noticed that CHANGES.md has not been updated. If this change updates the public API in any way, fixes a bug, or makes any non-trivial update, please add a bullet point to CHANGES.md and comment on this pull request so we know it was updated. For more info, see the Pull Request Guidelines.

I am a bot who helps you make Cesium awesome! Thanks again.

@shunter shunter force-pushed the velocityOrientation branch from 6f9e013 to 4111d8e Compare September 5, 2017 14:11
@emackey
Copy link
Contributor

emackey commented Sep 5, 2017

Awesome!

Is interpolation supported? I'm probably using it wrong, but I tried adding these lines to the aircraft position in your demo above:

      "interpolationAlgorithm":"LAGRANGE",
      "interpolationDegree":5,

The result was, the aircraft's orientation had both smooth interpolated changes cut by abrupt orientation changes at the path points. I would hope for just smooth changes, but having both is worse than having either one individually.

@shunter
Copy link
Contributor Author

shunter commented Sep 5, 2017

I'm assuming that those artifacts are just a result of limitations of the approach. Client-side finite differencing is not going to produce smooth results, since it's sort of trying to infer information that's not really part of the data.

@pjcozzi
Copy link
Contributor

pjcozzi commented Sep 6, 2017

@emackey are you volunteering to review this? 😄

@mramato
Copy link
Contributor

mramato commented Sep 7, 2017

I'm assuming that those artifacts are just a result of limitations of the approach. Client-side finite differencing is not going to produce smooth results, since it's sort of trying to infer information that's not really part of the data.

This could also be a data issue. I'm not sure where the data in this example came from, but it just might not lend itself well to Lagrange interpolation. The Interpolation Sandcastle example doesn't suffer from this problem.

@mramato
Copy link
Contributor

mramato commented Sep 7, 2017

At first glance, this all looks fine to me I'll take a closer look tomorrow but if someone beats me too it, that's fine by me.

Thanks @shunter!

@shunter shunter force-pushed the velocityOrientation branch 2 times, most recently from b79f98c to fad3e2e Compare September 11, 2017 14:52
* Improve handling of existing alignedAxis velocity support.  We now distinguish properties which must produce a unit cartesian (currently only alignedAxis).  For those properties, we always normalize on the client to avoid later errors.
* Fix velocityReference use from within an interval.  Fixes #5738
* Fix promise handling in CzmlDataSourceSpec.  Many tests were calling .load without using the resulting promise.  This meant that any exceptions during CZML load would be silently ignored.
@shunter shunter force-pushed the velocityOrientation branch from fad3e2e to 03b0a86 Compare September 26, 2017 14:00
@mramato
Copy link
Contributor

mramato commented Sep 27, 2017

Thanks again @shunter!

@mramato mramato merged commit e01f0b0 into master Sep 27, 2017
@mramato mramato deleted the velocityOrientation branch September 27, 2017 14:51
@mramato mramato removed their request for review October 12, 2017 14:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants