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

Apply styles to photogrammetry #7255

Merged
merged 2 commits into from
Nov 19, 2018
Merged

Apply styles to photogrammetry #7255

merged 2 commits into from
Nov 19, 2018

Conversation

lilleyse
Copy link
Contributor

@lilleyse 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
Copy link

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
Copy link
Contributor

pjcozzi commented Nov 11, 2018

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

@lilleyse
Copy link
Contributor Author

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

@OmarShehata
Copy link
Contributor

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
Copy link
Contributor Author

We could add more info to this issue #5550.

Basically tiles3d_tileset_time only actually works for point clouds right now.

@OmarShehata
Copy link
Contributor

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
Copy link
Contributor Author

I updated #5550.

@OmarShehata
Copy link
Contributor

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

@mramato
Copy link
Contributor

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"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@ggetz
Copy link
Contributor

ggetz commented Nov 16, 2018

Looks good other than that one comment.

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.

6 participants