3D Tiles Styling - Check for type mismatches in JS and shader backends #4860
Conversation
lilleyse
force-pushed
the
type-mismatch
branch
from
1b9566e to
4967e71
Compare
|
Some tests are broken and I need to write new tests. I don't want to get too far into this unless this is the right direction. |
| @@ -1060,8 +1104,17 @@ define([ | |||
| return Cartesian3.add(left, right, ScratchStorage.getCartesian3()); | |||
| } else if ((right instanceof Cartesian4) && (left instanceof Cartesian4)) { | |||
| return Cartesian4.add(left, right, ScratchStorage.getCartesian4()); | |||
| } else if ((typeof(left) === 'string') || (typeof(right) === 'string')) { | |||
| return left + right; | |||
There was a problem hiding this comment.
This relies on each types toString function. Should the spec be explicit about how each type is stringified?
There was a problem hiding this comment.
See what JavaScript says about toString functions. I want to say yes, we should state how, so things are very well specified, but we would also have to think about how many decimal places, etc.
| var left = this._left.evaluate(frameState, feature); | ||
| //>>includeStart('debug', pragmas.debug); | ||
| if (typeof(left) !== 'boolean') { | ||
| throw new DeveloperError('Error: Operation is undefined.'); |
There was a problem hiding this comment.
These exceptions are all fine, but can we be more precise about the type mismatch and any other info we can provide to help devs track down the issue?
There was a problem hiding this comment.
There's also need need to include Error: in the message, you are throwing an error so it's redundant.
Looks reasonable to me. Do you have any concerns? |
|
@lilleyse please bump when ready. |
lilleyse
force-pushed
the
type-mismatch
branch
from
07138a0 to
db4ed8e
Compare
lilleyse
force-pushed
the
type-mismatch
branch
from
db4ed8e to
091442e
Compare
|
Ready now. @Dylan-Brown you'll want to do similar type checking for the other built-in functions in #4865 |
|
Looks good! |
Based on discussion here: CesiumGS/3d-tiles#166
To start this goes through the unary and binary operators and throws errors if the arguments are not the same type. There's a bit more work to be done with the math library, vector constructors, etc.
In some ways this PR is starting the process of changing the styling language to not be as dependent on JavaScript type-conversion rules. Is the end goal of the spec to not rely on JavaScript conventions at all?