Refactor DynamicScene to use Geometry & Appearances #1444
Conversation
Start adding `isConstant` property and `definitionChanged` Event to all `Property` objects. Start migrating code to new DataSource directory.
It's no longer needed because it can be represented by standard properties. The code processing the CZML now lives in CzmlDataSource.
1. Get rid of `DynamicVertexPositionsProperty` and role the CZML processing aspect of it into CzmlDataSource. 2. Add `PropertyArray` and `PositionPropertyArray`, which allow for time-dynamic array properties. 3. Temporarily add some code to ReferenceFrameProperty to support positions, this may be removed in favor of a separate ReferencePositionProperty.
1. Add additional properties to DynamicEllipse, DynamicEllipsoid, and DynamicPolygon which will be used with new geometry code. 2. Tweaks to GeoJsonDataSource developers and minor CzmlDataSource cleanup.
It will be re-implemented using Geometry & Appearances.
Start of a new approach to taking advantange of Geometry & Appearances in DynamicScene. This manually ports some code from the abandoned dynamicScene-geometry branch and modifies it slightly for a new, cleaner approach. This is just the first baby stem to the process and lots of things don't work yet.
I also removed `outlineWidth` from various geometry options since it doesn't work on ANGLE devices and has batching limitations. Still needs a lot of cleanup.
1. Rename a bunch of variables to make the code easier to follow 2. Avoids iterating over all attributes every frame by keeping a separate map of time-dynamic updaters.
Still needs cleanup and ability to handle changed materials.
Also fix some failing specs.
Better support for show/fill/outline properties. Fix material handling and other misc cleanup.
Also some other minor fixes.
This is the start of larger set of changes to move to a more event driven graphics update loop, rather than the polling we do now.
`definitionChanged` is raised when any aspect of the object changes, i.e either by assigning a new property or modifying an existing property.
This is a breaking change which removes the practice of monkey patching primitives with a `dynamicObject` property.
| var show = new ShowGeometryInstanceAttribute(isAvailable && this._showProperty.getValue(time) && this._fillProperty.getValue(time)); | ||
| if (this._materialProperty instanceof ColorMaterialProperty) { | ||
| var currentColor = Color.WHITE; | ||
| if (defined(defined(this._materialProperty.color)) && (this._materialProperty.color.isConstant || isAvailable)) { |
There was a problem hiding this comment.
defined(defined(this._materialProperty.color))
pretty sure that's not what you mean to check. Same code in other updaters.
|
Okay, I think all current comments have been addressed. |
| */ | ||
| isConstant : { | ||
| get : function() { | ||
| return (!defined(this._color) || this._color.isConstant) && |
There was a problem hiding this comment.
Maybe we should make a helper function on Property or whatever to do !defined(x) || x.isConstant since we have this logic in a million places.
There was a problem hiding this comment.
Well, almost a million. 24. Regex:
!defined\((.+?)\) \|\| \1.isConstant
There was a problem hiding this comment.
Also check for the negation: defined\((.+?)\) && !\1.isConstant
There was a problem hiding this comment.
This is why we all want to be @shunter when we grow up.. unless you're just using ReSharper; then you're a fraud 😄
There was a problem hiding this comment.
Nope, I wrote those legit. :squirrel:
|
Okay, all pending comments have been addressed. |
| @@ -0,0 +1,113 @@ | |||
| /*global defineSuite*/ | |||
| defineSuite(['DynamicScene/PositionPropertyArray', | |||
There was a problem hiding this comment.
Typo in file name: PositionPropertyArraySepc
| var oldValue = this._value; | ||
| var simple = this._simple; | ||
| if ((simple && oldValue !== value) || (!simple && !oldValue.equals(value))) { | ||
| simple = typeof value !== 'object' || Array.isArray(value) || value instanceof Enumeration; |
There was a problem hiding this comment.
So because the default values for various properties are constructed as module-level objects, this code runs at module definition time. Everyone's favorite browser doesn't have Array.isArray, sadly, which brings everything crashing down.
Options:
- Make our own shim for
isArraylike we already do for various other things. It's easy to implement a shim withtoString. - Load es5-shim in CesiumViewer. This won't really remove the need our existing
definePropertiesandfreezeObjectshims, since neither can actually be shimmed correctly in ES3, so we just bail.
I think the first option is small enough to be OK. I'd rather not introduce more dependencies in Cesium Viewer if we don't need to.
| @@ -0,0 +1,22 @@ | |||
| /*global define,performance*/ | |||
There was a problem hiding this comment.
Copy/paste error. This is why we normally only have /*global define*/ at the top, and put other globals in another comment inside the setup function.
There was a problem hiding this comment.
I blame Kevin. All fixed.
| */ | ||
| GeometryVisualizer.prototype.update = function(time) { | ||
| if (!defined(time)) { | ||
| throw new DeveloperError('time is requied.'); |
There was a problem hiding this comment.
Needs a debug pragma. Also typo "requied".
There was a problem hiding this comment.
Turns out this typo has existed in several places for a while, now fixed.
…ctor Refactor DynamicScene to use Geometry & Appearances
There were regressions in the load code as part of #1444. Basically vertexPositions and directions can be arrays or objects, we were not handling the array case.
|
Revisiting this, it looks like the ellipsoid properties were never added to CzmlDataSource, so they've never worked. Specifically |
New Architecture overview
GeometryUpdateris a new interface which is implemented by a class that wishes to map one of theDynamicScenetypes to an equivalentGeometryInstanceandAppearance, i.e.DynamicPolygonis mapped to aPolygonGeometrybackedGeometryInstanceby the newPolygonGeometryUpdater. For static data, updaters themselves don't actually create primitives, they just create geometry instances which are analyzed and batched further up the chain. For dynamic data, eachGeometryUpdaterknows how to create aDynamicGeometryUpdaterinstance which manages a single primitive that represents that geometry from frame to frame.GeometryVisualizeris a newVisualizertype that handles batching and management of the updater supplied instances. It does not do much other than delegate to private helper classes,StaticGeometryColorBatch,StaticGeometryPerMaterialBatch,StaticOutlineGeometryBatch, andDynamicGeometryBatch(all of which should be self-explanatory). These helper classes actually create primitives and handle the binning of items (i.e. colored geometry is binned into translucent/opaque categories). GeometryVisualizer takes an Updater type in it's constructor, which means each visualizer only handles one type of geometry (by design). This works well because it makes it easy for each geometry type to have it's own appearance, for example, PolylineGeometryUpdater uses the PolylineColorAppearance, instead of the default PerInstanceColorAppearance.One design decision I made was to treat a filled and outlined geometry as either completely static or completely dynamic. This is less efficient in the case where one is static and the other isn't, but this almost never happens (for example, having a dynamic
numberOfVerticalLinesproperty for Ellipse, but static everything else). This decision basically makes the code a lot cleaner. We can always optimize it later if it becomes an issue. That was my approach overall, clean and complete first, optimize later.Static geometry is created asynchronously, while dynamic geometry is not. We also only evaluate/iterate over instance attributes that actually change from frame to frame. This is much more efficient than what is going on with the old visualizers in master.
Other DynamicScene changes
In order to support the above architecture, several changes and features had to be added to the underlying property system.
Properties now have an
isConstantproperty which means the value is constant as far as simulation time goes. The also have adefinitionChangedevent which fires if a mutable property is changed (for example, samples are added to SampledProperty). Both of these changes are necessary in order to properly batch and managed geometry and updates.DynamicPolygon/Ellipse/Ellipsoid/Polyline have new properties that mirror geometry options (
extrudedHeightfor example). Their visualizers have been replaced with updaters.The
propertyChangedevent on DynamicScene objects has been renamed todefinitionChanged. Also, the event is now raised in the case of an existing property being modified as well as having a new property assigned (previously only property assignment would raise the event).The
dynamicObjectproperty which got globbed onto all primitives for picking has been removed and replaced by using theidproperty, which is the correct way to do it anyway (and didn't exist when we first wrote this).DynamicDirectionsPropertyandDynamicVertexPositionsPropertywere both hold-overs from the originalDynamicPropertyimplementation. I finally got around to deleting them and replaced them withPropertyArrayandPositionPropertyArraywhich allows you to construct an array ofPropertyobjects that evaluates to an array whose values are the value of each property at that time. This is also ground work for aFormatterStringProperty, which will allow for time-dynamic strings with sampled values.TODO Before ready for final review