-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Visualizer cleanup #1653
Merged
+417
−1,330
Merged
Visualizer cleanup #1653
Changes from 8 commits
Commits
Show all changes
14 commits
Select commit
Hold shift + click to select a range
73bb9d0
Merge remote-tracking branch 'origin/issue1603' into visualizer-refactor
mramato 32f6125
Factor out Visualizer interface
mramato 416cecf
Merge branch 'dataSource-refactor' into kml
mramato 6578eb3
Merge branch 'master' into visualizer-refactor
mramato cb8e7d6
Ongoing visualizer cleanup.
mramato ffb06b3
Cleanup Visualizer-related documentation.
mramato d491e25
Update CHANGES
mramato a9241bb
Remove TODOs
mramato 58fd85c
Changes after review.
mramato e348b5d
Merge remote-tracking branch 'origin/master' into visualizer-refactor
mramato 2b0191f
Changes after review.
mramato 5a03804
Clean up whitespace.
shunter 5fbc9d7
Add back tests for isDestroy.
shunter 2169e97
Fix indentation.
shunter File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -29,12 +29,6 @@ define([ | |
MaterialProperty) { | ||
"use strict"; | ||
|
||
//CZML_TODO DynamicConeVisualizerUsingCustomSensor is a temporary workaround | ||
//because ComplexConicSensor has major performance issues. As soon as | ||
//ComplexConicSensor is working, this class can be deleted and | ||
//DynamicConeVisualizer is a drop in replacement that already does things | ||
//"the right way". | ||
|
||
var matrix3Scratch = new Matrix3(); | ||
|
||
function assignSpherical(index, array, clock, cone) { | ||
|
@@ -77,79 +71,36 @@ define([ | |
} | ||
|
||
/** | ||
* A DynamicObject visualizer which maps the DynamicCone instance | ||
* in DynamicObject.cone to a CustomSensor primitive. | ||
* A {@link Visualizer} which maps {@link DynamicObject#cone} to a {@link CustomSensor}. | ||
* @alias DynamicConeVisualizerUsingCustomSensor | ||
* @constructor | ||
* | ||
* @param {Scene} scene The scene the primitives will be rendered in. | ||
* @param {DynamicObjectCollection} [dynamicObjectCollection] The dynamicObjectCollection to visualize. | ||
* | ||
* @see DynamicCone | ||
* @see DynamicObject | ||
* @see DynamicObjectCollection | ||
* @see CompositeDynamicObjectCollection | ||
* @see DynamicBillboardVisualizer | ||
* @see DynamicConeVisualizer | ||
* @see DynamicLabelVisualizer | ||
* @see DynamicPointVisualizer | ||
* @see DynamicPyramidVisualizer | ||
* @param {DynamicObjectCollection} dynamicObjectCollection The dynamicObjectCollection to visualize. | ||
*/ | ||
var DynamicConeVisualizerUsingCustomSensor = function(scene, dynamicObjectCollection) { | ||
//>>includeStart('debug', pragmas.debug); | ||
if (!defined(scene)) { | ||
throw new DeveloperError('scene is required.'); | ||
} | ||
if (!defined(dynamicObjectCollection)) { | ||
throw new DeveloperError('dynamicObjectCollection is required.'); | ||
} | ||
//>>includeEnd('debug'); | ||
|
||
dynamicObjectCollection.collectionChanged.addEventListener(DynamicConeVisualizerUsingCustomSensor.prototype._onObjectsRemoved, this); | ||
|
||
this._scene = scene; | ||
this._unusedIndexes = []; | ||
this._primitives = scene.primitives; | ||
this._coneCollection = []; | ||
this._dynamicObjectCollection = undefined; | ||
this.setDynamicObjectCollection(dynamicObjectCollection); | ||
}; | ||
|
||
/** | ||
* Returns the scene being used by this visualizer. | ||
* | ||
* @returns {Scene} The scene being used by this visualizer. | ||
*/ | ||
DynamicConeVisualizerUsingCustomSensor.prototype.getScene = function() { | ||
return this._scene; | ||
}; | ||
|
||
/** | ||
* Gets the DynamicObjectCollection being visualized. | ||
* | ||
* @returns {DynamicObjectCollection} The DynamicObjectCollection being visualized. | ||
*/ | ||
DynamicConeVisualizerUsingCustomSensor.prototype.getDynamicObjectCollection = function() { | ||
return this._dynamicObjectCollection; | ||
}; | ||
|
||
/** | ||
* Sets the DynamicObjectCollection to visualize. | ||
* | ||
* @param dynamicObjectCollection The DynamicObjectCollection to visualizer. | ||
*/ | ||
DynamicConeVisualizerUsingCustomSensor.prototype.setDynamicObjectCollection = function(dynamicObjectCollection) { | ||
var oldCollection = this._dynamicObjectCollection; | ||
if (oldCollection !== dynamicObjectCollection) { | ||
if (defined(oldCollection)) { | ||
oldCollection.collectionChanged.removeEventListener(DynamicConeVisualizerUsingCustomSensor.prototype._onObjectsRemoved, this); | ||
this.removeAllPrimitives(); | ||
} | ||
this._dynamicObjectCollection = dynamicObjectCollection; | ||
if (defined(dynamicObjectCollection)) { | ||
dynamicObjectCollection.collectionChanged.addEventListener(DynamicConeVisualizerUsingCustomSensor.prototype._onObjectsRemoved, this); | ||
} | ||
} | ||
this._dynamicObjectCollection = dynamicObjectCollection; | ||
}; | ||
|
||
/** | ||
* Updates all of the primitives created by this visualizer to match their | ||
* Updates the primitives created by this visualizer to match their | ||
* DynamicObject counterpart at the given time. | ||
* @memberof DynamicLabelVisualizer | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thought I caught all of these. Thanks. |
||
* | ||
* @param {JulianDate} time The time to update to. | ||
* @returns {Boolean} This function always returns true. | ||
|
@@ -171,62 +122,25 @@ define([ | |
}; | ||
|
||
/** | ||
* Removes all primitives from the scene. | ||
* Removes and destroys all primitives created by this instance. | ||
* @memberof DynamicLabelVisualizer | ||
*/ | ||
DynamicConeVisualizerUsingCustomSensor.prototype.removeAllPrimitives = function() { | ||
var i, len; | ||
for (i = 0, len = this._coneCollection.length; i < len; i++) { | ||
this._primitives.remove(this._coneCollection[i]); | ||
DynamicConeVisualizerUsingCustomSensor.prototype.destroy = function() { | ||
var dynamicObjectCollection = this._dynamicObjectCollection; | ||
dynamicObjectCollection.collectionChanged.removeEventListener(DynamicConeVisualizerUsingCustomSensor.prototype._onObjectsRemoved, this); | ||
|
||
var i; | ||
var dynamicObjects = dynamicObjectCollection.getObjects(); | ||
var length = dynamicObjects.length; | ||
for (i = 0; i < length; i++) { | ||
dynamicObjects[i]._coneVisualizerIndex = undefined; | ||
} | ||
|
||
if (defined(this._dynamicObjectCollection)) { | ||
var dynamicObjects = this._dynamicObjectCollection.getObjects(); | ||
for (i = dynamicObjects.length - 1; i > -1; i--) { | ||
dynamicObjects[i]._coneVisualizerIndex = undefined; | ||
} | ||
length = this._coneCollection.length; | ||
for (i = 0; i < length; i++) { | ||
this._primitives.remove(this._coneCollection[i]); | ||
} | ||
|
||
this._unusedIndexes = []; | ||
this._coneCollection = []; | ||
}; | ||
|
||
/** | ||
* Returns true if this object was destroyed; otherwise, false. | ||
* <br /><br /> | ||
* If this object was destroyed, it should not be used; calling any function other than | ||
* <code>isDestroyed</code> will result in a {@link DeveloperError} exception. | ||
* | ||
* @memberof DynamicConeVisualizerUsingCustomSensor | ||
* | ||
* @returns {Boolean} True if this object was destroyed; otherwise, false. | ||
* | ||
* @see DynamicConeVisualizerUsingCustomSensor#destroy | ||
*/ | ||
DynamicConeVisualizerUsingCustomSensor.prototype.isDestroyed = function() { | ||
return false; | ||
}; | ||
|
||
/** | ||
* Destroys the WebGL resources held by this object. Destroying an object allows for deterministic | ||
* release of WebGL resources, instead of relying on the garbage collector to destroy this object. | ||
* <br /><br /> | ||
* Once an object is destroyed, it should not be used; calling any function other than | ||
* <code>isDestroyed</code> will result in a {@link DeveloperError} exception. Therefore, | ||
* assign the return value (<code>undefined</code>) to the object as done in the example. | ||
* | ||
* @memberof DynamicConeVisualizerUsingCustomSensor | ||
* | ||
* @returns {undefined} | ||
* | ||
* @exception {DeveloperError} This object was destroyed, i.e., destroy() was called. | ||
* | ||
* @see DynamicConeVisualizerUsingCustomSensor#isDestroyed | ||
* | ||
* @example | ||
* visualizer = visualizer && visualizer.destroy(); | ||
*/ | ||
DynamicConeVisualizerUsingCustomSensor.prototype.destroy = function() { | ||
this.setDynamicObjectCollection(undefined); | ||
return destroyObject(this); | ||
}; | ||
|
||
|
@@ -280,7 +194,6 @@ define([ | |
dynamicObject._coneVisualizerIndex = coneVisualizerIndex; | ||
cone.id = dynamicObject; | ||
|
||
// CZML_TODO Determine official defaults | ||
cone.material = Material.fromType(Material.ColorType); | ||
cone.intersectionColor = Color.clone(Color.YELLOW); | ||
cone.intersectionWidth = 5.0; | ||
|
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should keep
isDestroyed
throughout since it's part of thedestroy
"interface".There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I got rid of it because Visualizers are not general purpose objects and their lifetime is completely managed by
DataSourceDisplay
. I could have renameddestroy
to something else, but I didn't see the point. AddingisDestroyed
here feels like consistency for the sake of consistency taken to far with no real benefit.Now that's I've said my peace, if I didn't convince you I can put it back; but it just feels pointless.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not convinced. These would be the first and only non-private classes in Cesium to break convention, and the private ones should be fixed. Imagine we have a
Destroyable
interface with those two methods. Maybe we should make one for documentation purposes.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I'll put them back, but I think we really need to re-think our object lifetime strategy on the whole anyway.