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

Apply styles to photogrammetry #7255

Merged
merged 2 commits into from Nov 19, 2018

Conversation

Projects
None yet
6 participants
@lilleyse
Contributor

lilleyse commented Nov 11, 2018

Fixes #7149

Styles now work for tilesets that don't have features. All property expressions (e.g. ${Height}) evaluate to undefined.

@OmarShehata can you review?

tileset.style = new Cesium.Cesium3DTileStyle({
    color : 'rgba(255, 0, 0, 0.5)'
});

style

@cesium-concierge

This comment has been minimized.

cesium-concierge commented Nov 11, 2018

Thanks for the pull request @lilleyse!

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

🌍 🌎 🌏

@pjcozzi

This comment has been minimized.

Member

pjcozzi commented Nov 11, 2018

@lilleyse does this require a spec update - either normative or implementation note?

@lilleyse

This comment has been minimized.

Contributor

lilleyse commented Nov 12, 2018

After reading the styling page, yes it does require normative update. I opened a spec PR here: AnalyticalGraphicsInc/3d-tiles#352.

@OmarShehata

This comment has been minimized.

Contributor

OmarShehata commented Nov 13, 2018

This is awesome @lilleyse ! I can run my original transparency demo from the forum thread now. All tests are passing, and works in Chrome, Edge and IE11.

The only thing I feel uneasy about is:

function evaluateTilesetTime(feature) {
    if (!defined(feature)) {
        return 0.0;
    }
    return feature.content.tileset.timeSinceLoad;
}

Since that means photogrammetry wouldn't be able to use time-based styles, and it would be kind of opaque to the user why this doesn't work. This built in time variable is only documented in the spec, and it would be strange to add to add this CesiumJS specific implementation issue there.

I guess getting that to work would require passing in the original tileset to the evaluate function? Is that a difficult refactor? It might be worth at least opening up an issue for.

@lilleyse

This comment has been minimized.

Contributor

lilleyse commented Nov 13, 2018

We could add more info to this issue #5550.

Basically tiles3d_tileset_time only actually works for point clouds right now.

@OmarShehata

This comment has been minimized.

Contributor

OmarShehata commented Nov 13, 2018

Oh! Yeah I think in that case, just update that issue saying that it also won't work when there are no features, and I think this is good to merge.

I can bump the forum thread once it's merged.

@lilleyse

This comment has been minimized.

Contributor

lilleyse commented Nov 13, 2018

I updated #5550.

@OmarShehata

This comment has been minimized.

Contributor

OmarShehata commented Nov 14, 2018

You have to merge this yourself by the way @lilleyse .

@mramato

This comment has been minimized.

Member

mramato commented Nov 14, 2018

@ggetz please do a final review and merge. Thanks.

expression = new Expression('${feature["vector"]}');
expect(expression.evaluate(undefined)).toBeUndefined();
// Evaluating inside a string is an exception. "" is returned instead of "undefined"

This comment has been minimized.

@ggetz

ggetz Nov 16, 2018

Contributor

Why is this an exception? It looks like we determine the behavior in the previous test for what do when printing an undefined property.

This comment has been minimized.

@lilleyse

lilleyse Nov 16, 2018

Contributor

Oh just an exception in reference to the name of the test "evaluates variable to undefined if feature is undefined"

@ggetz

This comment has been minimized.

Contributor

ggetz commented Nov 16, 2018

Looks good other than that one comment.

@ggetz ggetz merged commit 1ce4134 into master Nov 19, 2018

7 checks passed

continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
deployment Deployed
Details
npm package Deployed
Details
zip file Deployed
Details

@ggetz ggetz deleted the apply-style-to-photogrammetry branch Nov 19, 2018

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