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 support for PolylineVolume in CZML #8841

Merged
merged 7 commits into from
Jun 15, 2020

Conversation

jrafidi
Copy link
Contributor

@jrafidi jrafidi commented May 12, 2020

Fixes #3062

Did my best to port forward #7175. I'm somewhat fuzzy around the reference properties and time intervals and tried my best to follow other examples in the file. Please let me know if there are other test cases I should add or any other obvious issues.

CLA signing is in progress.

@cesium-concierge
Copy link

Thanks for the pull request @jrafidi!

  • ❌ Missing CLA.
  • CHANGES.md was not updated.
    • If this change updates the public API in any way, please add a bullet point to CHANGES.md.

Reviewers, don't forget to make sure that:

  • Cesium Viewer works.
  • Works in 2D/CV.
  • Works (or fails gracefully) in IE11.

@hpinkos
Copy link
Contributor

hpinkos commented May 12, 2020

Thanks @jrafidi! We'll review this once we get your CLA

@jrafidi
Copy link
Contributor Author

jrafidi commented May 15, 2020

@hpinkos I believe the CLA has been submitted. Have you received it on your end?

@hpinkos
Copy link
Contributor

hpinkos commented May 18, 2020

Got it, thanks @jrafidi! I'll have time to review this later this week.

@jrafidi
Copy link
Contributor Author

jrafidi commented May 26, 2020

@hpinkos Do you have time to take a look this week? Would be nice to have this in the next release.

@hpinkos
Copy link
Contributor

hpinkos commented May 26, 2020

@jrafidi sorry, I had a deadline for something else creep up on me. I won't have time today, but hopefully before the end of the week?

@mramato @shunter would you have a minute to review this?

@hpinkos
Copy link
Contributor

hpinkos commented May 29, 2020

@jrafidi sorry, it looks like we won't be able to get this in for the June release, but I'll make sure to review it in time for the July release. Thanks for your understanding

@hpinkos
Copy link
Contributor

hpinkos commented Jun 15, 2020

great work on this, thanks @jrafidi!

fyi @shunter

@shunter
Copy link
Contributor

shunter commented Feb 15, 2021

Finally taking a look at trying to add this to the CZML spec. Note that in CZML "cartesian" exclusively refers to a three-dimensional value X, Y, Z, where "cartesian2" exclusively refers to a two-dimensional value X, Y. So the "shape" of the volume should have been defined as a "cartesian2" list value.

I think for backwards compatibility we can keep the erroneous "cartesian" property support. There are other cases like this where we still support old incorrect syntax.

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.

Add PolylineVolume to CZML
4 participants