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

Initial support for AGI_articulations #7835

Merged
merged 12 commits into from May 30, 2019
Merged

Initial support for AGI_articulations #7835

merged 12 commits into from May 30, 2019

Conversation

emackey
Copy link
Contributor

@emackey emackey commented May 13, 2019

This PR has initial support for 3D model articulations from the AGI_articulations vendor extension to glTF 2.0. There is no pointing vector or attach point support here, just articulations.

This isn't polished yet, but I'm opening it for early review from @shunter, @abwood, and @gbeatty.

TODO

  • There should be a better way to expose the available articulation names, stage name, min, max, and get/set current stage value. Let's push this until a more complete solution for 3D Model Entity API at a disadvantage #7831 is designed.
  • Add unit tests.
  • Possibly in a separate PR, add support at the Entity layer. Also pushed.

@cesium-concierge
Copy link

cesium-concierge commented May 13, 2019

Thanks for the pull request @emackey!

  • ✔️ Signed CLA found.
  • CHANGES.md was not updated.
    • If this change updates the public API in any way, please add a bullet point to CHANGES.md.
  • ❔ Unit tests were not updated.
    • Make sure you've updated tests to reflect your changes, added tests for any new code, and ran the code coverage tool.

Reviewers, don't forget to make sure that:

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

Source/Scene/Model.js Outdated Show resolved Hide resolved
@emackey
Copy link
Contributor Author

emackey commented May 13, 2019

Here's a live demo.

CesiumArticulations

@TJKoury
Copy link
Contributor

TJKoury commented May 23, 2019

This is awesome!

@emackey
Copy link
Contributor Author

emackey commented May 23, 2019

The scope of this PR has been dialed in a little. It's write-only (same as nodeTransformations) support for articulations at the model primitive level only.

Ready for full review & merge.

@emackey
Copy link
Contributor Author

emackey commented May 28, 2019

Any chance this can be merged in time for release? Scott doesn't know the contents of Model.js well enough to be comfortable merging this himself. We're confident in the articulations functionality, just need to make sure we're not breaking some conventions or established patterns in that file.

@lilleyse
Copy link
Contributor

I took a quick look at the Model.js changes. They look totally fine to me. Should this change be mentioned in CHANGES.md?

@emackey
Copy link
Contributor Author

emackey commented May 30, 2019

CHANGES.md updated, thanks.

@emackey
Copy link
Contributor Author

emackey commented May 30, 2019

@shunter Can you do the honors, or does this need additional reviewers?

Source/Scene/Model.js Outdated Show resolved Hide resolved
Specs/Scene/ModelSpec.js Outdated Show resolved Hide resolved
@abwood
Copy link
Contributor

abwood commented May 30, 2019

The application of articulation transformations looks to be consistent with STK's implementation. Here's the sand castle example using facility.glb:
image

and here are those same articulations applied to the default facility in STK:
image

Copy link
Contributor

@abwood abwood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good on my end once the Travis CI issue is adjudicated.

@emackey
Copy link
Contributor Author

emackey commented May 30, 2019

Thanks! Travis is a fluke, some unrelated test flaked out.

@shunter shunter merged commit 8a7939f into master May 30, 2019
@shunter shunter deleted the articulations branch May 30, 2019 17:32
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.

None yet

6 participants