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 visibility for non-dynamic show #7156

Merged
merged 6 commits into from
Oct 22, 2018
Merged

Fix entity visibility for non-dynamic show #7156

merged 6 commits into from
Oct 22, 2018

Conversation

hpinkos
Copy link
Contributor

@hpinkos hpinkos commented Oct 16, 2018

Fixes #7132

Updating the show property has a special case if show is constant. Instead of checking the value of the attribute every frame, we listen for changes to the isShowing property and update the attribute value without creating a new primitive. The the old polygon was showing when the new polygon was added because we reused the geometry instance that still had the old attribute value when the new primitive was created with both geometry instances.

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

var greenPolygon = viewer.entities.add({
    polygon : {
        hierarchy : Cesium.Cartesian3.fromDegreesArray([-108.0, 42.0,
                                                        -100.0, 42.0,
                                                        -104.0, 40.0]),
        material : Cesium.Color.GREEN,
        height: 0
    }
});


Sandcastle.addToolbarButton('Hide', function(){
    greenPolygon.show = false;
});

Sandcastle.addToolbarButton('Add polygon', function(){
    viewer.entities.add({
        polygon : {
            hierarchy : {
                positions : Cesium.Cartesian3.fromDegreesArray([-108.0, 33.0,
                                                                -100.0, 33.0,
                                                                -104.0, 35.0]),
            },
            material : Cesium.Color.RED,
            height: 0
        }
    });
});

@cesium-concierge
Copy link

Thanks for the pull request @hpinkos!

  • ✔️ 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.

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.

🌍 🌎 🌏

@hpinkos
Copy link
Contributor Author

hpinkos commented Oct 16, 2018

I tested to make sure this didn't introduce a regression for any of these cases: #7053 #7048 #2909 #4714 #6830 #5371

@mramato
Copy link
Contributor

mramato commented Oct 18, 2018

This all sounds vaguely familiar to me, but do we know when this regression happened? This is just putting back old behavior, right?

@mramato
Copy link
Contributor

mramato commented Oct 18, 2018

git bisect points to 6d11d0f as the first bad commit. Does that change how this should be fixed at all?

@hpinkos
Copy link
Contributor Author

hpinkos commented Oct 18, 2018

This is a better fix than the old behavior because it specifically addresses changes to show for static geometry. The code from that commit you mentioned was removed because it was causing problems for dynamic geometry #7048

@mramato
Copy link
Contributor

mramato commented Oct 18, 2018

Sounds good. Can we add unit tests?

@hpinkos
Copy link
Contributor Author

hpinkos commented Oct 18, 2018

Thanks for pushing back on this @mramato! I assumed we didn't have many unit tests for things like this because they were difficult or impossible to write, but it was actually pretty straight forwards. This is ready now

@mramato
Copy link
Contributor

mramato commented Oct 22, 2018

You have some failing tests

@hpinkos
Copy link
Contributor Author

hpinkos commented Oct 22, 2018

@mramato this should be ready now

@mramato
Copy link
Contributor

mramato commented Oct 22, 2018

Travis builds are down while GitHub is still recovering from earlier failure. So I'll have to test this locally but will merge if there aren't any problems.

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.

3 participants