-
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
Remove handling of deprecated scene parameter in constructor #4514
Remove handling of deprecated scene parameter in constructor #4514
Conversation
@erikmaarten this is exactly what was needed. Can you add a line to CHANGES.md to mention this? After that I'll merge. Thanks. |
You can also update the code in |
@mramato Added to CHANGES.md now |
@bagnell I'm not quite sure about this, but it looks like the if statement could be removed, and
could be run for every initializer, since these must now have a cluster, and will not have a scene. Is that right? |
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. |
Thanks @bagnell. About the tests, they actually pass without changes. MockVisualizer should probably take an |
The |
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! |
For issue #4385. @bagnell Does this look OK?