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

Greatly improve performance of clamped Entity geometry with dynamic colors #8630

Merged
merged 3 commits into from Feb 26, 2020

Conversation

@mramato
Copy link
Member

mramato commented Feb 25, 2020

When we originally added support for GroundPrimitive, it did not support per-instance colors. We fixed this some time ago but apparently the Entity API or ClassificationPrimitive was never properly updated to take advantage of it.

The below Sandcastle code is unusably slow in master, in this branch is flies.

const viewer = new Cesium.Viewer('cesiumContainer', {
    terrainProvider: Cesium.createWorldTerrain()
});

const color = new Cesium.CallbackProperty(() => {
    return Cesium.Color.fromRandom({
        alpha: 1
    });
}, false);

const granularity = 3;
for (let lon = -180.0; lon < 180.0; lon += granularity) {
    for (let lat = -90.0; lat < 90.0; lat += granularity) {
        const entity = viewer.entities.add(new Cesium.Entity({
            rectangle: {
                coordinates: Cesium.Rectangle.fromDegrees(lon, lat, lon + granularity, lat + granularity),
                material: new Cesium.ColorMaterialProperty(color)
            }
        }));
    }
}

console.log(`Created ${viewer.entities.values.length} entities`);

I have some failing tests that need to be fixed, but I wanted to get the PR open to ensure that there wasn't some other reason this limitation was in place that I missed. Especially my changes to the ClassificationPrimitive and Primitive. CC @likangning93 @lilleyse

…olors

When we originally added support for GroundPrimitive, it did not support
per-instance colors. We fixed this some time ago but apparently the Entity
API or ClassificationPrimitive was never properly updated to take advantage
of it.
@mramato mramato requested review from lilleyse, hpinkos and likangning93 Feb 25, 2020
@cesium-concierge

This comment has been minimized.

Copy link

cesium-concierge commented Feb 25, 2020

Thanks for the pull request @mramato!

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

This comment has been minimized.

Copy link
Member Author

mramato commented Feb 25, 2020

Assuming no surprises, it would be nice to get this into the next reelase.

@hpinkos

This comment has been minimized.

Copy link
Contributor

hpinkos commented Feb 25, 2020

Looks great to me @mramato!

@mramato

This comment has been minimized.

Copy link
Member Author

mramato commented Feb 26, 2020

Turns out the tests were an easy fix, so CI should be happy now. @lilleyse what are your thoughts here?

@lilleyse

This comment has been minimized.

Copy link
Contributor

lilleyse commented Feb 26, 2020

👍 from me

@lilleyse lilleyse merged commit 49bdead into master Feb 26, 2020
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@lilleyse lilleyse deleted the faster branch Feb 26, 2020
mramato added a commit that referenced this pull request Mar 19, 2020
The performance improvements #8630 did not take into account the limitation
that ground primitives that overlap cannot be batched together.

This change uses the `RectangleCollisionChecker` already implemented for
`StaticGroundGeometryMaterialBatch` and performs the same check in
`StaticGroundGeometryColorBatch`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.