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

Fix entity show issues, issue #7048 #7053

Merged
merged 6 commits into from
Sep 21, 2018
Merged

Fix entity show issues, issue #7048 #7053

merged 6 commits into from
Sep 21, 2018

Conversation

cleroux
Copy link
Contributor

@cleroux cleroux commented Sep 20, 2018

Addresses issue #7048
Fixes a regression caused by #6835 which resolved issues #2909, #4714, #6830, and #5371.

Reverted #6835 locally and confirmed each issue.
Applied these changes and confirmed correct behavior for each of the listed issues.
Ran all tests before and after changes. No new test failures reported.
Additionally ran many of the Sandcastle examples and observed no abnormal behavior.

@cesium-concierge
Copy link

Thank you so much for the pull request @cleroux! I noticed this is your first pull request and I wanted to say welcome to the Cesium community!

The Pull Request Guidelines is a handy reference for making sure your PR gets accepted quickly, so make sure to skim that.

Reviewers, don't forget to make sure that:

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

I am a bot who helps you make Cesium awesome! Contributions to my configuration are welcome.

🌍 🌎 🌏

@cleroux
Copy link
Contributor Author

cleroux commented Sep 20, 2018

My individual CLA has been sent.

@pjcozzi
Copy link
Contributor

pjcozzi commented Sep 20, 2018

Thanks again @cleroux, we received your CLA.

@hpinkos
Copy link
Contributor

hpinkos commented Sep 20, 2018

Thanks for the PR @cleroux! I'm glad you were able to track down the source of the problem. As you mentioned in your comment in #7048, that code was there for a reason, but I can't remember exactly what off the top of my head. I'll take a closer look so we can merge this fix in for the next release without introducing a new regression.

@hpinkos
Copy link
Contributor

hpinkos commented Sep 21, 2018

@cleroux upon further review, I think your change is the right thing to do here. I believe this used to work differently, but pretty much what's happening now is that block you removed was setting the attribute value of the new geometry instance to the attributes of the old geometry instance which is removed when the new one is ready. This is what was causing the flickering. However, at the end of the block we blow away all of the attribute values anyway so there was no point in updating them. https://github.com/AnalyticalGraphicsInc/cesium/blob/master/Source/DataSources/StaticGeometryColorBatch.js#L190

Thanks so much for tracking this down!

@hpinkos hpinkos merged commit 5352b53 into CesiumGS:master Sep 21, 2018
@OmarShehata
Copy link
Contributor

This also fixes the crash described here:

https://groups.google.com/d/msg/cesium-dev/1LHJQatvBZk/jbobj_-SCQAJ

which is reproduced by:

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

viewer.screenSpaceEventHandler.setInputAction(function onLeftClick(movement) {
     viewer.entities.getById("1").polygon.material.color = Cesium.Color.BLUE;
}, Cesium.ScreenSpaceEventType.LEFT_CLICK);

var redPolygon = viewer.entities.add({
    name : 'Red polygon on surface',
    id: '1',
    polygon : {
        hierarchy : Cesium.Cartesian3.fromDegreesArray([-115.0, 37.0,
                                                        -115.0, 32.0,
                                                        -107.0, 33.0,
                                                        -102.0, 31.0,
                                                        -102.0, 35.0]),
        material : Cesium.Color.RED
    }
});

for(var i=0;i<100;i++){
viewer.entities.add({
    polygon : {
        hierarchy : Cesium.Cartesian3.fromDegreesArray([-115.0, 37.0 + i,
                                                        -115.0, 32.0 + i,
                                                        -107.0, 33.0 + i,
                                                        -102.0, 31.0 + i,
                                                        -102.0, 35.0 + i]),
        material : Cesium.Color.GREEN
    }
});
}

redPolygon.show = false;
setTimeout(function(){
    redPolygon.show = true;
},10000);

viewer.zoomTo(viewer.entities);

which I was just about to open an issue for, so thanks again @cleroux !

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.

5 participants