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

Don't share geometry arrays between Batch and Primitive #8569

Merged
merged 3 commits into from
Jan 28, 2020

Conversation

shunter
Copy link
Contributor

@shunter shunter commented Jan 28, 2020

Make a copy of the array of geometry instances before passing to the Primitive constructor.

In the batches, geometries is a live reference to the array inside the AssociativeArray, which is owned by the batch. We need to make a copy of the array so that subsequent changes to the batch don't affect the Primitive's view of the instances, which could happen during async creation or combining.

Fixes #5947

I marked this next release because the crash in #5947 is consistently affecting a customer application so I would like to get this looked at for 1.66.

…Primitive constructor.

In the batches, the `geometries` is a live reference to the array inside the AssociativeArray owned by the batch. We need to make a copy of the array so that subsequent changes to the batch don't affect the Primitive's view of the instances, which could happen during async creation or combining.
@cesium-concierge
Copy link

Thanks for the pull request @shunter!

  • ✔️ Signed CLA found.
  • CHANGES.md was not updated.
    • If this change updates the public API in any way, please add a bullet point to CHANGES.md.
  • ❔ Unit tests were not updated.
    • Make sure you've updated tests to reflect your changes, added tests for any new code, and ran the code coverage tool.

Reviewers, don't forget to make sure that:

  • Cesium Viewer works.
  • Works in 2D/CV.
  • Works (or fails gracefully) in IE11.

@shunter
Copy link
Contributor Author

shunter commented Jan 28, 2020

Here's the Sandcastle test case copied from #5947

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

var entities = [];

var i;
for (i = 0; i < 100; ++i) {
    entities.push(viewer.entities.add({
        polyline: {
        positions : Cesium.Cartesian3.fromDegreesArray([-75, i/2,
                                                        -125, i/4])
        }
    }));
}

function removeEntity() {
  console.log(viewer.entities.removeById(entities[entities.length-1].id));    
}

var originalPrimitiveUpdate = Cesium.Primitive.prototype.update;

var updateCount = 0;

Cesium.Primitive.prototype.update = function(frameState) {
    originalPrimitiveUpdate.apply(this, arguments);
    
    if (updateCount === 0) {
        removeEntity();
    }
   
    updateCount++;
};

@mramato
Copy link
Contributor

mramato commented Jan 28, 2020

Thanks @shunter

Will look at this right now.

@mramato
Copy link
Contributor

mramato commented Jan 28, 2020

By right now, I mean this afternoon of course.

@mramato
Copy link
Contributor

mramato commented Jan 28, 2020

Reading the linked issue and trying to job my memory, but I can't think of a single reason we would ever have intentionally shared these arrays. I think making a copy is the right things to do here.

@mramato
Copy link
Contributor

mramato commented Jan 28, 2020

@shunter just to throw out another thought, would it make more sense for Primitive to copy during construction? I guess this is kind of a CesiumJS design decision we made a long time ago for performance reasons, we never copy arrays unless absolutely needed.

So if that's still the pattern we follow, I'll merge this. Just figured it was worth a separate comment first.

@mramato
Copy link
Contributor

mramato commented Jan 28, 2020

Update CHANGES

@shunter
Copy link
Contributor Author

shunter commented Jan 28, 2020

I thought about putting the defensive copy inside the Primitive constructor, but that would introduce an extra redundant copy in the numerous cases where we are passing a temporary array into the constructor.

@shunter
Copy link
Contributor Author

shunter commented Jan 28, 2020

I updated CHANGES.

@mramato
Copy link
Contributor

mramato commented Jan 28, 2020

Yeah, that's what I figured. Thanks @shunter!

@mramato mramato merged commit 8648a46 into master Jan 28, 2020
@mramato mramato deleted the dontShareGeometryArray branch January 28, 2020 20:36
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.

TypeError: Cannot set property 'geometry' of undefined
3 participants