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

Clone NearFarScalar values in constructor for Billboard, Label and PointPrimitive #5654

Merged
merged 1 commit into from
Jul 19, 2017

Conversation

hpinkos
Copy link
Contributor

@hpinkos hpinkos commented Jul 19, 2017

In master, these two billboards have different behavior when you click the update button because the scaleByDistance setter clones the NearFarScalar but the constructor does not. This updates the constructor so it also clones the value passed in.

var viewer = new Cesium.Viewer('cesiumContainer');
var scene = viewer.scene;

var scalar = new Cesium.NearFarScalar(1.5e2, 2.0, 1.5e7, 0.5);

var billboards1 = scene.primitives.add(new Cesium.BillboardCollection());
var b1 = billboards1.add({
    image : '../images/facility.gif',
    position : Cesium.Cartesian3.fromDegrees(-75, 40),
    scaleByDistance : scalar
});

var billboards2 = scene.primitives.add(new Cesium.BillboardCollection());
var b2 = billboards2.add();
b2.image = '../images/facility.gif';
b2.position = Cesium.Cartesian3.fromDegrees(-76, 40);
b2.scaleByDistance = scalar;

Sandcastle.addToolbarButton('Update', function() {
    scalar.far = 1.5e5;
    b1.scaleByDistance = scalar;
    b2.scaleByDistance = scalar;
});

@mramato
Copy link
Contributor

mramato commented Jul 19, 2017

As a counter argument, should we be cloning at all? Isn't this extremely memory inefficient if you want to share the same values across all billboards?

@hpinkos
Copy link
Contributor Author

hpinkos commented Jul 19, 2017

In all of the primitives I just looked at, any time we pass in a Cartesian3 or Matrix4 or other objects we always clone them. Just for consistency alone, I think we should make this change. If we want to revisit 'should we really be cloning' it should be across the board.

@bagnell bagnell merged commit 5a95263 into master Jul 19, 2017
@bagnell bagnell deleted the clone-near-far-scalars branch July 19, 2017 18:46
@hpinkos
Copy link
Contributor Author

hpinkos commented Jul 19, 2017

Thanks @bagnell!

@bagnell
Copy link
Contributor

bagnell commented Jul 19, 2017

As a counter argument, should we be cloning at all? Isn't this extremely memory inefficient if you want to share the same values across all billboards?

As far as I know, we clone all options to a constructor/setter. This prevents users from constructing with one object, changing the value and constructing another object.

@pjcozzi
Copy link
Contributor

pjcozzi commented Jul 19, 2017

Sounds like this fixes a bug-ish / changes behavior.

Please update CHANGES.md in master.

@hpinkos
Copy link
Contributor Author

hpinkos commented Jul 20, 2017

Done 8bc1760

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants