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

CZML model node transformations #3316

Merged
merged 24 commits into from
Dec 18, 2015
Merged

CZML model node transformations #3316

merged 24 commits into from
Dec 18, 2015

Conversation

shunter
Copy link
Contributor

@shunter shunter commented Dec 8, 2015

This builds on top of Greg's changes in #3003 and includes all of them.

I'm opening this for initial review, though I still need to add some additional tests.

czml-writer pr: AnalyticalGraphicsInc/czml-writer#102

This adds support for time-varying model node transformations in CZML. A NodeTransformation aggregates scale, translation and rotation as values. NodeTransformationProperty produces NodeTransformation values from properties.

ModelGraphics now has nodeTransformations which is a new PropertyBag object, which is a key-value store for properties, including definitionChanged propagation. PropertyBag may be useful for eventually supporting custom CZML properties.

@shunter
Copy link
Contributor Author

shunter commented Dec 8, 2015

With Greg's help, I put together an Sandcastle example that shows this working.

var viewer = new Cesium.Viewer('cesiumContainer');

viewer.dataSources.add(Cesium.CzmlDataSource.load('https://gist.githubusercontent.com/shunter/08c95fcebb1c3506530a/raw/09805419206c60eea4cf8948c056a92ae29b15c8/satellite.czml')).then(function(dataSource) {
    viewer.trackedEntity = dataSource.entities.getById('Satellite/Satellite2');
});

@pjcozzi
Copy link
Contributor

pjcozzi commented Dec 8, 2015

It would be nice to include a CZML example for this in Sandcastle like the other CZML examples.

Also, I was not planning on reviewing this unless there is something in particular you guys want me to look at.

Also, do you guys want to write a short tech blog post on CZML/glTF interop? There would be interest from the Cesium and glTF communities.

@shunter
Copy link
Contributor Author

shunter commented Dec 9, 2015

For the sandcastle example, I figure we probably don't want to commit the 4MB gltf file from STK referenced in my previous example above. Let me know if you think this is too dumb looking for a demo. I wanted to make the transformations visually obvious.

http://shunter.github.io/nodeTransformations/Apps/Sandcastle/gallery/CZML%20Model%20-%20Node%20Transformations.html

@pjcozzi
Copy link
Contributor

pjcozzi commented Dec 9, 2015

I don't think users will realize that the example is correct. Perhaps just spin the Cesium Milk truck 360 degrees - or if you really want to emphasize the "we're referencing a node id, not the entire model aspect", @lilleyse could create a simple model with two nodes for this.

@shunter
Copy link
Contributor Author

shunter commented Dec 9, 2015

Yeah, I think we need to emphasize that it affects nodes, and not the whole model (spinning a whole model is trivial without any of these changes). I'm happy to use a different model instead.

@pjcozzi
Copy link
Contributor

pjcozzi commented Dec 9, 2015

@lilleyse can you create something simple for @shunter?

@lilleyse
Copy link
Contributor

What about raising and lowering the body of the milk truck, like if someone was changing the suspension? Or maybe rotating the wheels of the truck to look like a turn?

* @param {Cartesian3} [translation=Cartesian3.ZERO] A {@link Cartesian3} specifying the (x, y, z) translation to apply to the node.
* @param {Quaternion.IDENTITY} [rotation=Quaternion.IDENTITY] A {@link Quaternion} specifying the (x, y, z, w) rotation to apply to the node.
*/
var NodeTransformation = function(scale, translation, rotation) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I understand why this is in DataSources, but it certainly looks like something I would except to be in Core. Are we sure that we're never going to need something like this at the lower level?

Copy link
Contributor

Choose a reason for hiding this comment

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

Core is probably a better choice. Probably name it Transform, AffineTransform, or TranslationRotationScale.

Throughout, the parameters should also be in that order: translation, rotation, scale. TRS, as it is called, is common in graphics and what we say in glTF.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, renamed to TranslationRotationScale and moved to Core.

@shunter
Copy link
Contributor Author

shunter commented Dec 11, 2015

regarding node transformations, I put together a sandcastle demo that allows direct manipulation of node transforms in the UI.

http://shunter.github.io/modelNodes/3D%20Model%20Node%20Explorer.html

you can make Cesium Man do a little dance for you by rotating joints.

I'm thinking this could be useful to go into the development directory. opinions?

@pjcozzi
Copy link
Contributor

pjcozzi commented Dec 12, 2015

The development directory is OK, but most developers using models would be interested in this so I would suggest putting it in the main Sandcastle examples.

@pjcozzi
Copy link
Contributor

pjcozzi commented Dec 12, 2015

There will also be a separate CZML example, right?

@shunter
Copy link
Contributor Author

shunter commented Dec 15, 2015

I left the Model Node Explorer example in the development section because 1) it does not use the entity API and 2) it uses private Cesium members (it's not possible to enumerate model nodes in the public API).

I also added the CZML example. Cesium Man says hello: http://shunter.github.io/nodeTransformations/CZML%20Model%20-%20Node%20Transformations.html

@pjcozzi
Copy link
Contributor

pjcozzi commented Dec 15, 2015

Amazing.

@mramato
Copy link
Contributor

mramato commented Dec 15, 2015

Also, as soon as this is merged and @gbeatty's exporter is updated, we are going to replace the C4ISR example from the website with an awesome model-based simulation. I also want to look into making a separate space-based example, perhaps with the IIS or maneuvering satellites.

@mramato
Copy link
Contributor

mramato commented Dec 18, 2015

We totally need a demo of the Cesium man doing the robot ;)

var defaultRotation = Quaternion.IDENTITY;

/**
* A set of transformations to apply to a particular node in a {@link Model}.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is kind of more generic now, right? @pjcozzi do you have a suggestion for a one-line definition of TRS?

Copy link
Contributor

Choose a reason for hiding this comment

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

An affine transformation defined by a translation, rotation, and scale.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks.

Add `Matrix4.fromTranslationRotationScale`.
Remove `TranslationRotationScale.prototype.toMatrix`
Tweak documentation
Get rid of empty function
Update tests.
@mramato
Copy link
Contributor

mramato commented Dec 18, 2015

My changes are in and I also updated CHANGES with the full list (it's actually quite a bit).

Awesome work @gbeatty and @shunter. This is an often requested chunk of functionality and will make a lot of people very happy.

mramato added a commit that referenced this pull request Dec 18, 2015
…sformations

CZML model node transformations
@mramato mramato merged commit 403b01f into master Dec 18, 2015
@mramato mramato deleted the czmlModelNodeTransformations branch December 18, 2015 21:12
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

5 participants