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

Add isNaN, isFinite, null, and undefined support to GLSL styling backend #8621

Merged
merged 1 commit into from Feb 19, 2020

Conversation

@lilleyse
Copy link
Contributor

lilleyse commented Feb 19, 2020

Fixes #8615

Local sandcastle

The goal of this PR is to make isNaN, isFinite, null, and undefined usable in point clouds styles so that a style written for 3D buildings with null properties won't throw an error if also applied to point clouds.

  • isNaN - isnan is not available in GLSL 1.0 so the approximation is value != value. The Sandcastle confirms that this check works.
  • isFinite - isinf is not available in GLSL 1.0 so the approximation is value < czm_infinity. GLSL doesn't have an infinity constant and floating point precision is not predictable enough across the board to use the actual max float. czm_infinity is 5906376272000.0, the distance from the Sun to Pluto in meters, and is "big enough".
  • null - uses sentinel czm_infinity. Would have preferred to use NaN but GLSL doesn't have a NaN constant. You can sometimes fake a NaN by doing an operation like sqrt(-20.0) but I read that it's not reliable across all drivers/GPUs.
  • undefined - uses sentinel czm_infinity. Same as above.

This PR does not attempt to make GLSL follow the same casting and comparison rules as JavaScript. E.g. Boolean(null) is false in JS while bool(5906376272000.0) is true in transcoded GLSL. Not to mention all the math operations and edge cases like isNaN(null). I think it's possible to fix nearly all of these with some ternaries like for Boolean(value) getting transcoded to '(' + left + ' === ' + nullSentinel + ' ? false : bool(' + left + ')', and I started doing that, but it's not super important and was holding up this PR.

But in the end I'm not too worried about this because per-point properties are stored in binary and can never be null or undefined. The user would need to write a meaningless style like ${temperature} < 10.0 === Boolean(undefined) to trigger one of the edge cases.

@lilleyse lilleyse requested a review from OmarShehata Feb 19, 2020
@lilleyse lilleyse force-pushed the glsl-null-undefined branch from 581031b to 54f71be Feb 19, 2020
@mramato

This comment has been minimized.

Copy link
Member

mramato commented Feb 19, 2020

Awesome, thanks @lilleyse!

@OmarShehata I modified your example to use the extra check from Sean's demo: link

Just to summarize to make sure I understand it all, it's still possible to write styles that work in a JS back-end but not in a glsl backend, but now it's actually possible to write 100% generic styles that can work with any datasets and backed, regardless of null/NaN/undefined values in the data. You just need to take care to guard against null/undefined/nan properties in the styles themselves.

Basically, for numeric styles this boils down to a top level condition like:

['isNaN(${Year}) || !isFinite(${Year}) || ${Year} === null || ${Year} === undefined', 'color("blue")'],

Though in non-numeric properties, it would be a bit simpler.

Anyway, code looks straightforward enough and good to me. @OmarShehata please merge if you're happy as well.

Thanks again, @lilleyse!

@lilleyse

This comment has been minimized.

Copy link
Contributor Author

lilleyse commented Feb 19, 2020

Just to summarize to make sure I understand it all, it's still possible to write styles that work in a JS back-end but not in a glsl backend, but now it's actually possible to write 100% generic styles that can work with any datasets and backed, regardless of null/NaN/undefined values in the data. You just need to take care to guard against null/undefined/nan properties in the styles themselves.

Numeric data should be good now but strings and regular expressions will still throw in the GLSL backend. It's harder problem... let me know if this will come up in the near future.

Example style that will throw for point clouds:

['${Name} === "BuildingA"', 'color("blue")'],
['${Name} === "BuildingB"', 'color("green")'],
['true', 'color("red")']

("blue" and "green" are fine but "BuildingA" and "BuildingB" are what triggers the error)

@OmarShehata

This comment has been minimized.

Copy link
Contributor

OmarShehata commented Feb 19, 2020

@lilleyse so just to confirm, you're saying ${Name} === "BuildingA" will crash if this is a GLSL backend (like in a point cloud) and Name is null? Could I as a user guard against that in my styling with a isNaN or value != value check?

Going to go ahead and merge because this fixes the issues we were seeing, thanks for the quick fix Sean!

@OmarShehata OmarShehata merged commit b80f2e8 into master Feb 19, 2020
6 checks passed
6 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage results Deployed
Details
deployment Deployed
Details
npm package Deployed
Details
zip file Deployed
Details
@OmarShehata OmarShehata deleted the glsl-null-undefined branch Feb 19, 2020
@lilleyse

This comment has been minimized.

Copy link
Contributor Author

lilleyse commented Feb 19, 2020

@OmarShehata unfortunately any usage of string literals or regex's will throw, even something simple that doesn't use property names like show : '"name1" !== "name2"'.

There's a lot of questions like how do strings get passed to GLSL, how does a function like toString work in GLSL, how would we go about implementing regex's in GLSL? Is there some subset of behavior we can support...

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.

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