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

Remove handling of deprecated scene parameter in constructor #4514

Merged

Conversation

erikmaarten
Copy link
Contributor

@erikmaarten erikmaarten commented Oct 24, 2016

For issue #4385. @bagnell Does this look OK?

@mramato
Copy link
Contributor

mramato commented Oct 24, 2016

@erikmaarten this is exactly what was needed. Can you add a line to CHANGES.md to mention this? After that I'll merge. Thanks.

@bagnell
Copy link
Contributor

bagnell commented Oct 24, 2016

You can also update the code in DataSourceDisplay._onDataSourceAdded and DataSourceDisplay._onDataSourceRemoved that checks for clustering on the visualizers.

@erikmaarten
Copy link
Contributor Author

@mramato Added to CHANGES.md now

@erikmaarten
Copy link
Contributor Author

@bagnell I'm not quite sure about this, but it looks like the if statement could be removed, and

cluster._initialize(scene);
scene.primitives.add(cluster);

could be run for every initializer, since these must now have a cluster, and will not have a scene. Is that right?
Such a change breaks a lot of tests, which seems to be related to a MockVisualizer in DataSourceDisplaySpec.js not yet being adapted to new the new Visualizer shape, but I'm not sure I should make too big changes to this code yet since I'm not very familiar with how the visualizers work.

@bagnell
Copy link
Contributor

bagnell commented Oct 25, 2016

You should be able to remove the loop when the data source is added because

var visualizers = this._visualizersCallback(scene, entityCluster, dataSource);
dataSource._visualizers = visualizers;

should construct the visualizers with the cluster that was just initialized. The loop when a data source is remove only needs to destroy the visualizers. Yes, the tests in DataSourceDisplaySpec need to be updated, but I think the other tests should be fine.

@erikmaarten
Copy link
Contributor Author

Thanks @bagnell. About the tests, they actually pass without changes. MockVisualizer should probably take an entityCluster instead of scene as the first argument, but MockVisualizer is never called with arguments so it shouldn't make a difference. Is this what's intended?

@fredj
Copy link
Contributor

fredj commented Oct 26, 2016

The deprecationWarning "include" in the file define (sorry, probably not the right term) can be removed in all the file where the deprecationWarning call where removed

@mramato
Copy link
Contributor

mramato commented Oct 26, 2016

Thanks @fredj I pushed a change to remove them.

@erikmaarten since they were unused, I just removed the two constructor parameters from MockVisualizer. If we ever need the Mock to do more, we can add them back (my pipe dream is still to get webgl working on travis so we can actually mock less stuff, but that's a PR for another time).

This looks good to me. Thanks @erikmaarten and @fredj!

@mramato mramato merged commit 567f1a4 into CesiumGS:master Oct 26, 2016
@erikmaarten erikmaarten deleted the remove-deprecated-parameter-handling branch October 26, 2016 15:38
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.

None yet

4 participants