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

Refactor DyanmicScene Properties #1080

Merged
merged 93 commits into from
Aug 29, 2013
Merged

Refactor DyanmicScene Properties #1080

merged 93 commits into from
Aug 29, 2013

Conversation

mramato
Copy link
Contributor

@mramato mramato commented Aug 26, 2013

This is a huge pull request that technically has many breaking changes, but in reality; almost all of our users' own code will be unaffected (for example, no sandcastle examples needed to be modified). This change is stage 1 of the DynamicScene & DataSource roadmap (#1045) and vastly improves the underlying property system used by DynamicScene. The monolithic DynamicProperty has been split into many smaller purpose-built objects that make writing DataSources much easier.

This pull request is all about infrastructure, there is no change to current functionality other than exposing new API and removing some obsolete ones. Stage 2 will consist of rewriting the Visualizers to take advantage of the new property system and greatly optimize rendering, by both reducing the amount of work done for static objects as well as taking alternate rendering paths when Geometry and Appearances can be used. This will make static data like GeoJSON and KML fly, and should also provide improvements to dynamic demos like Lots of Satellites.

I don't care if this goes into b20. I even started a b21 for CHANGES because I figured it will take a while for this to merge. I know @shunter has already been looking at this, @emackey this should barely affect the code in your app (if at all), but if you want to try it out and confirm that before we merge as a beta test, that would be great.

Directly from CHANGES

  • Breaking changes:
    • Removed processCzml, use CzmlDataSource instead.
    • Completely refactored the DynamicScene property system. See #1080 for complete details.
      • Removed CzmlBoolean, CzmlCartesian2, CzmlCartesian3, CzmlColor, CzmlDefaults, CzmlDirection, CzmlHorizontalOrigin, CzmlImage, CzmlLabelStyle, CzmlNumber, CzmlPosition, CzmlString, CzmlUnitCartesian3, CzmlUnitQuaternion, CzmlUnitSpherical, and CzmlVerticalOrigin since they are no longer needed.
      • Removed DynamicProperty, DynamicMaterialProperty, DynamicDirectionsProperty, and DynamicVertexPositionsProperty; replacing them with an all new system of properties.
        • Property - base interface for all properties.
        • CompositeProperty - a property composed of other properties.
        • ConstantProperty - a property whose value never changes.
        • SampledProperty - a property whose value is interpolated from a set of samples.
        • TimeIntervalCollectionProperty - a property whose value changes based on time interval.
        • MaterialProperty - base interface for all material properties.
        • CompositeMaterialProperty - a CompositeProperty for materials.
        • ColorMaterialProperty - a property that maps to a color material. (replaces DynamicColorMaterial)
        • GridMaterialProperty - a property that maps to a grid material. (replaces DynamicGridMaterial)
        • ImageMaterialProperty - a property that maps to an image material. (replaces DynamicImageMaterial)
        • PositionProperty- base interface for all position properties.
        • CompositePositionProperty - a CompositeProperty for positions.
        • ConstantPositionProperty - a whose value does not change in respect to the ReferenceFrame in which is it defined.
        • SampledPositionProperty - a SampledProperty for positions.
        • TimeIntervalCollectionPositionProperty - A TimeIntervalCollectionProperty for positions.
  • Added Packable and PackableForInterpolation interfaces to aid interpolation and in-memory data storage. Also made most core Cesium types implement them.
  • Added InterpolationAlgorithm interface to codify the base interface already being used by LagrangePolynomialApproximation, LinearApproximation, and HermitePolynomialApproximation.

mramato added 30 commits June 21, 2013 12:14
1. Remove `getValueCartographic`
2. Rename `getValueCartesian` to getValue.
3. Remove `getValueRangeCartesian` since it is unused.
4. Also start to clean up DyanmicDirectionsProperty.
…or DynamicDirectionsProperty.

In reality, I might abandon what I've done so far in favor of a new approach.
Conflicts:
	Source/DynamicScene/DynamicConeVisualizerUsingCustomSensor.js
DynamicPositionProperty is still using it, but that will change shortly.
Conflicts:
	Source/DynamicScene/ConstantProperty.js
	Source/DynamicScene/CzmlUnitSpherical.js
	Source/DynamicScene/DynamicBillboard.js
	Source/DynamicScene/DynamicBillboardVisualizer.js
	Source/DynamicScene/DynamicColorMaterial.js
	Source/DynamicScene/DynamicCone.js
	Source/DynamicScene/DynamicDirectionsProperty.js
	Source/DynamicScene/DynamicEllipse.js
	Source/DynamicScene/DynamicEllipsoid.js
	Source/DynamicScene/DynamicGridMaterial.js
	Source/DynamicScene/DynamicImageMaterial.js
	Source/DynamicScene/DynamicLabel.js
	Source/DynamicScene/DynamicLabelVisualizer.js
	Source/DynamicScene/DynamicObject.js
	Source/DynamicScene/DynamicObjectView.js
	Source/DynamicScene/DynamicPath.js
	Source/DynamicScene/DynamicPoint.js
	Source/DynamicScene/DynamicPointVisualizer.js
	Source/DynamicScene/DynamicPolygon.js
	Source/DynamicScene/DynamicPolyline.js
	Source/DynamicScene/DynamicPolylineVisualizer.js
	Source/DynamicScene/DynamicPositionProperty.js
	Source/DynamicScene/DynamicPyramid.js
	Source/DynamicScene/DynamicPyramidVisualizer.js
	Source/DynamicScene/DynamicVector.js
	Source/DynamicScene/DynamicVertexPositionsProperty.js
	Source/DynamicScene/ReferenceProperty.js
	Specs/MockProperty.js
1. Move some of the specs to SampledPropertySpec.
2. Temporarily comment out path visualizer.
3. Use a regular property for now instead of position properties.
1. Introduce InterpolatableValue interface and make most basic types we support implement it.
2. Add InterpolatableNumber.
3. Update SampledProperty to use the InterpolatableValue interface rather than specialized helper classes.
1. Delete a lot of now dead code in preparation of bigger CZML parsing changes.
2. Move `processCzml` to `CzmlDataSource.processCzml` for now, but we may get rid of it entirely.
Instead, use a mix of native JavaScript and Core types.
return vectorUpdated;
}

var updaters = [processClock,//
Copy link
Contributor

Choose a reason for hiding this comment

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

In the current state, there's no easy way to add additional updaters, since the public API doesn't provide any way to pass updaterFunctions to _processCzml. I guess for now we can just attach this array of defaults to CzmlDataSource to take the place of the former CzmlDefaults.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was an oversight on my part. I agree we can expose CzmlDataSource.updaters for now, and I suppose we can also expose all of the updater functions as well so that people can add/remove them. I also forgot to update changes to mention this.

@mramato
Copy link
Contributor Author

mramato commented Aug 27, 2013

First round of changes are in. All current review comments have been addressed.

this._xTable = [];
this._yTable = [];
this._packedInterpolationLength = packedInterpolationLength;
this._updateTables = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

updateTableLength or updateTableSize would be clearer, I think, since we always update the tables' contents regardless.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed.

@shunter
Copy link
Contributor

shunter commented Aug 27, 2013

This looks fine to me. Anyone else want to review? Otherwise, I see no need to delay this past next week's release.

@mramato
Copy link
Contributor Author

mramato commented Aug 27, 2013

We'll need to update CHANGES, but otherwise we can merge this if you don't see any reason not to.

@cmorse
Copy link
Contributor

cmorse commented Aug 27, 2013

@mramato Thanks for doing this! Makes the czml processing code a lot easier to understand.

@mramato
Copy link
Contributor Author

mramato commented Aug 27, 2013

Thanks @cmorse It's nice to know we're headed in the right direction. We have more big changes planned to hopefully make the whole thing easier to use. I want it to be trivial for users to easily get there data into Cesium, whether they are using CZML or not. The next big pass is going to deal with optimizing visualizers so we can use alternate rendering paths automatically based on the type of data the user supplies.

@mramato
Copy link
Contributor Author

mramato commented Aug 28, 2013

@shunter I updated changes to everything is in b20. This is ready whenever you want to merge it.

@cmorse
Copy link
Contributor

cmorse commented Aug 29, 2013

@mramato That's great to hear. The new CesiumViewer class and this commit are a big step towards it being much easier to get working with Cesium.

shunter added a commit that referenced this pull request Aug 29, 2013
@shunter shunter merged commit 29e6656 into master Aug 29, 2013
@shunter shunter deleted the dynamicScene-properties branch August 29, 2013 20:47
mramato added a commit that referenced this pull request Sep 3, 2013
#1080 accidentally removed some helper functions needed for custom CZML processing.  I expect these function to go away in the near future, but they are definitely needed for now in apps that use custom CZML.

CC @emackey
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.

3 participants