Support for entities as reference frames

[speigg]

This PR is based on the referenceFrames branch, and adds support for constructing trees of entities by allowing entities to be used as reference frames themselves (see #1045). The root reference frame for any tree is either FIXED, INERTIAL, or an entity with an undefined position (an arbitrary frame).

For example:

var e1 = new Entity();
e1.position = new ConstantPositionProperty(new Cartesian3(100000, 200000, 300000)); // implicit FIXED frame
var orientation = new Quaternion(0, 0, 1, 1);
e1.orientation = new ConstantProperty(Quaternion.normalize(orientation, orientation));

var e2 = new Entity();
e2.position = new ConstantPositionProperty(new Cartesian3(200, 400, 600), e1); // note: e1 here
orientation = new Quaternion(1, 0, 0, 1);
e2.orientation = new ConstantProperty(Quaternion.normalize(orientation, orientation));

var value = new Cartesian3(1, 2, 3);
var result = PositionProperty.convertToReferenceFrame(time, value, e2, ReferenceFrame.FIXED);

expect(result).toEqual(new Cartesian3(99603, 200201, 300602));

// Can also pass an arbitrary Entity as the referenceFrame parameter in getValueInReferenceFrame().
// The reference frame can be any Entity... if both entities are not connected, the result is undefined. 
e1.position.getValueInReferenceFrame(time, e2) 

PositionProperty.convertToReferenceFrame has been rewritten to support converting positions between any two reference frames, including any two entities. Likewise, OrientationProperty.convertToReferenceFrame has been added to convert orientations between any two reference frames.

ReferenceEntity class is added to support the '#id' notation for entities as reference frames in CzmlDataSource when the specified Entity does not yet exist in the EntityCollection (https://github.com/AnalyticalGraphicsInc/cesium/wiki/CZML-Content#position).

Specs have been added for PositionProperty and OrientationProperty.
JSHint passes.

Summary of Changes:

• Modified CzmlDataSource to process ‘#id’
• Added OrientationProperty (convertToReferenceFrame)
• Modified PositionProperty (convertToReferenceFrame)
• Added ReferenceEntity
• Added OrientationPropertySpec
• Fixed PositionPropertySpec tests in the referenceFrames branch
• Added OrientationPropertySpec

Features not included:

• getValueInReferenceFrame does not yet exist for the orientation property on the Entity class (the more cumbersome OrientationProperty.convertToReferenceFrame has to be used to convert orientations between reference frames instead). I think this would require adding many new *OrientationProperty classes, the equivalent of all the *PositionProperty classes, unless we remove the existing *PositionProperty classes and just add getValueInReferenceFrame to the generic *Property classes.
• The ability to specify a reference frame for an orientation other than the reference frame specified by the position property.

[pjcozzi]

Thanks for the pull request! Just a heads up that a lot of the Cesium team is out of the office for the next week or so.

Everyone - we have a CLA for @speigg.

[speigg]

Can someone review this please? Thanks :)

[speigg]

@pjcozzi any estimate on when this PR will be reviewed? Thanks!

[pjcozzi]

@shunter has been doing a lot of CZML work recently.

@shunter is reviewing this PR of interest to you?

[speigg]

@pjcozzi The most important changes are in the PositionProperty / OrientationProperty. The changes to CzmlDataSource and (the added) ReferenceEntity are needed only for CZML support, but I can take these out and leave them for a separate PR if it would make this easier to review. Perhaps @mramato should review this since I built on-top of his original commits for the PR?

[pjcozzi]

@speigg yes, @mramato is the right person to review this. It might have to be after the holidays.

[speigg]

Latest merge from master caused jshint errors. I'll fix these soon.

[pjcozzi]

@speigg any update here?

[speigg]

HI @pjcozzi, sorry for the delay, I will fix this today.

[speigg]

Okay, I've fixed the build, hopefully CI agrees once it completes.
I added a couple details to the description above. Namely highlighting how this PR allows for the getValueInReferenceFrame function to work as expected when passing an Entity instance as the reference frame:

// Can also pass an arbitrary Entity as the referenceFrame parameter in getValueInReferenceFrame().
// The reference frame can be any Entity... if both entities are not connected, the result is undefined. 
e1.position.getValueInReferenceFrame(time, e2) 

This is what remains to be implemented for this reference frames feature to be fully completed:

• getValueInReferenceFrame does not yet exist for the orientation property on the Entity class (the more cumbersome OrientationProperty.convertToReferenceFrame has to be used to convert orientations between reference frames instead). I think this would require adding many new *OrientationProperty classes, the equivalent of all the *PositionProperty classes, unless we remove the existing *PositionProperty classes and just add getValueInReferenceFrame to the generic *Property classes.
• CZML support for specifying orientations relative to other reference frames

[speigg]

Ah, I fixed the conflicts but forgot to check the jshint errors. Will try again...

[speigg]

Okay, all checks passed.

One other thing I wanted to point out is an inconsistency in the current API:

• getValue on a position property always returns position value relative to the FIXED frame
• getValue on an orientation property returns position value relative to the position's reference frame

This inconsistency is more unexpected once arbitrary entities are allowed as reference frames. Especially once both the orientation and position properties have a getValueInReferenceFrame method, I would expect getValue to either return the local value, or the FIXED value, but not sometimes one and other times the second. Unfortunately, making the getValue api consistent would probably mean a breaking change for any apps that ever expect orientation.getValue to return the orientation in the INERTIAL frame instead of FIXED.

[speigg]

@pjcozzi Also, I asked this earlier but never got a response, please let me know whether or not I should remove the CZML related changes, and save those for a separate PR later. Maybe that would make this easier to review... :)

[pjcozzi]

Thanks for the prompt updates, @speigg!

@mramato any chance you can carve out a few minutes to review this?

[shunter]

also fixes #1319

[speigg]

I realized the other day I can simplify the code a little. No need to support null as a reference frame, since it's functionally the same as using an entity with an undefined position property (or even an entity with a position property which returns an undefined position value).

[speigg]

Alright, I made a minor change to simplify the code a little bit as I described in my previous comment.

[speigg]

So, any chance on this getting merged? This PR's birthday is coming up :)

[hpinkos]

Thanks for your patience @speigg!
Sorry, we've been really busy lately. We'll try to find some time to review this soon.

[shunter]

I've started looking at the changes here, and I have initial concerns about the performance of the implementation. It wouldn't be unusual to expect this function to be called 500,000+ times/sec. Currently there are a number of array and closure allocations on every invocation, which I suspect would produce significant GC churn.

In order to keep Cesium's performance acceptable in a web browser, we typically have to write what I usually call "the stupidest code" we can, i.e. plain for loops, as well as staying as close as we can to zero-allocation per frame, hence all our scratch variables and result parameters. Sadly, this often precludes writing smart functional code, e.g. reduce, shift/unshift, slice, etc. as stupid code typically beats the performance of smart code by orders of magnitude.

This is also probably a section of code that would benefit from memoization - rather than computing the chain of transformations every time the function is called, that chain information can be cached and reused for a given input and output frame, with careful control of cache lifetime so it can't grow unbounded.

I will continue to look through this as I have time. We do want this functionality in Cesium.

[speigg]

Thanks for taking the time to review this PR @shunter! My main focus was on functionality, so I am happy to hear that this implementation is at least functionally in line with planned features in Cesium.

I believe most of the allocations can be eliminated, and I agree that the use of memoization would be ideal here. Let me know if you would like me to work on reducing some of these allocations. At the very least, it should be easy to add a "fast path" which maintains the current performance metrics for the current use case of converting between FIXED and INERTIAL frames.

[pjcozzi]

@shunter @speigg is there plan to continue work from this pull request or should we close this?

[shunter]

@pjcozzi This would still be a good addition, but as I mentioned earlier, it needs a rethink from a performance perspective. I don't know when I'll have time to look at this myself.

[speigg]

I can create the "fast path" as I mentioned so that performance remains equivalent for at least the current use-cases. Eliminating allocations should be straightforward too. The trickiest part would be the memoization, I think, but as long as the scene graph remains shallow, memoization won't provide a huge benefit anyways. Memoization can be added later in another PR, I think.

[pjcozzi]

@speigg do you plan to make the optimizations you suggested above?

We are cleaning up outstanding pull requests so if you don't plan to make this change in perhaps the next month or so, we can close this and write an issue.

[speigg]

@pjcozzi sure, I will make the optimizations I described.

[pjcozzi]

@speigg is this something you still plan to work on soon or should we close this for now and perhaps move the details to an issue?

[speigg]

I’ve been meaning to get to this sooner, but it’s up to you. I can always open a new PR when it’s ready.

[pjcozzi]

@speigg OK, let's close this just so we tidy up our open pull requests a bit. We look forward to your update!