Add ordering at the DataSource level #6316
Conversation
|
@hpinkos, thanks for the pull request! Maintainers, we have a signed CLA from @hpinkos, so you can review this at any time.
I am a bot who helps you make Cesium awesome! Contributions to my configuration are welcome. 🌍 🌎 🌏 |
|
|
||
| this._dataSourceCollection = dataSourceCollection; | ||
| this._scene = scene; | ||
| this._primitives = scene.primitives.add(new PrimitiveCollection()); |
There was a problem hiding this comment.
Can we avoid adding these primitives until at least one data source is added? Otherwise users will always have these two primitives in there app, which is something we try and avoid.
There was a problem hiding this comment.
Actually, this doesn't get us anything because we add the defaultDataSource in the constructor
There was a problem hiding this comment.
That's unfortunate. Are you sure there's no way for us to create the primitives but not add them until something is actually added to the scene?
| @@ -90,7 +95,7 @@ define([ | |||
| DataSourceDisplay.defaultVisualizersCallback = function(scene, entityCluster, dataSource) { | |||
| var entities = dataSource.entities; | |||
| return [new BillboardVisualizer(entityCluster, entities), | |||
| new GeometryVisualizer(scene, entities), | |||
| new GeometryVisualizer(scene, dataSource), | |||
There was a problem hiding this comment.
This is another breaking change, correct? (I'm okay with it, just making sure we document it).
There was a problem hiding this comment.
Actually,why not just pass the primitives and ground primitive collections instead of the dataSource? This keeps it loosely coupled.
There was a problem hiding this comment.
I was trying to cut down on the number of params, but I'm okay passing entites, primitives, groundPrimitives
|
Works as advertised, those two comments were all I had as far as feedback on the approach. When this is ready for merge I'll do a more thorough review. Thanks @hpinkos! |
|
Ready for a full review |
|
I'll have to figure out why all these tests are failing on travis, they pass locally |
|
No worries, just bump when it's ready and I'll do a full review. |
| collection.raise(source); | ||
| collection.lower(source); | ||
| collection.raiseToTop(source); | ||
| collection.lowerToBottom(source); |
There was a problem hiding this comment.
We try to avoid uber tests like this that call a whole bunch of functions at once because they are hard to debug when something brakes. If you think this is the best way to test the ordering functionality, we can keep this. But please add comments to each function call indicating what exactly is being tested.
|
Just those two comments. |
|
@mramato ready
|
|
Thanks @hpinkos! |
This change makes order matter in
DataSourceCollection. It only has an effect on ground primitives. All ground primitives from oneDataSourcerender above all ground primitives in a lowerDataSourcein the sameDataSourceCollectionraise,lower,raiseToTop,lowerToButtommethods inDataSourceCollectionscene.primitives/scene.groundPrimitives,DataSourceDisplaycreates it's ownPrimitiveCollections. This is so the order of data sources in the data source collection has a one-to-one mapping with the order of primitives in the primitive collection.DataSourceDisplaycreates aPrimitiveCollectionfor all the entities in that data source to use.DataSourceDisplayre-orders the correspondingPrimitiveCollectionso the ground primitives layer accordingly