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.

Sign up for GitHub

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

3D Tiles Styling - Check for type mismatches in JS and shader backends #4860

Merged
merged 4 commits into from
Jan 17, 2017
Merged

3D Tiles Styling - Check for type mismatches in JS and shader backends #4860

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4967e71
Check for type mismatch in JS and shader backends
lilleyse Jan 12, 2017
4f79f98
Merge branch '3d-tiles' into type-mismatch
lilleyse Jan 16, 2017
c81c4cf
Added more detailed error messages
lilleyse Jan 16, 2017
091442e
Added tests
lilleyse Jan 16, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
101 changes: 91 additions & 10 deletions Source/Scene/Expression.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -960,7 +960,13 @@ define([
// that we can assign if we know the types before runtime

Node.prototype._evaluateNot = function(frameState, feature) {
return !(this._left.evaluate(frameState, feature));
var left = this._left.evaluate(frameState, feature);
//>>includeStart('debug', pragmas.debug);
if (typeof(left) !== 'boolean') {
throw new DeveloperError('Error: Operation is undefined.');
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

There's also need need to include Error: in the message, you are throwing an error so it's redundant.

}
//>>includeEnd('debug');
return !left;
};

Node.prototype._evaluateNegative = function(frameState, feature) {
Expand All @@ -971,39 +977,77 @@ define([
return Cartesian3.negate(left, ScratchStorage.getCartesian3());
} else if (left instanceof Cartesian4) {
return Cartesian4.negate(left, ScratchStorage.getCartesian4());
} else if (typeof(left) === 'number') {
return -left;
}
return -left;

//>>includeStart('debug', pragmas.debug);
throw new DeveloperError('Error: Operation is undefined.');
//>>includeEnd('debug');
return -left; // jshint ignore:line
};

Node.prototype._evaluatePositive = function(frameState, feature) {
var left = this._left.evaluate(frameState, feature);
if ((left instanceof Cartesian2) || (left instanceof Cartesian3) || (left instanceof Cartesian4)) {
return left;

//>>includeStart('debug', pragmas.debug);
if (!((left instanceof Cartesian2) || (left instanceof Cartesian3) || (left instanceof Cartesian4) || (typeof(left) === 'number'))) {
throw new DeveloperError('Error: Operation is undefined.');
}
return +left;
//>>includeEnd('debug');

return left;
};

Node.prototype._evaluateLessThan = function(frameState, feature) {
var left = this._left.evaluate(frameState, feature);
var right = this._right.evaluate(frameState, feature);

//>>includeStart('debug', pragmas.debug);
if ((typeof(left) !== 'number') || (typeof(right) !== 'number')) {
throw new DeveloperError('Error: Operation is undefined.');
}
//>>includeEnd('debug');

return left < right;
};

Node.prototype._evaluateLessThanOrEquals = function(frameState, feature) {
var left = this._left.evaluate(frameState, feature);
var right = this._right.evaluate(frameState, feature);

//>>includeStart('debug', pragmas.debug);
if ((typeof(left) !== 'number') || (typeof(right) !== 'number')) {
throw new DeveloperError('Error: Operation is undefined.');
}
//>>includeEnd('debug');

return left <= right;
};

Node.prototype._evaluateGreaterThan = function(frameState, feature) {
var left = this._left.evaluate(frameState, feature);
var right = this._right.evaluate(frameState, feature);

//>>includeStart('debug', pragmas.debug);
if ((typeof(left) !== 'number') || (typeof(right) !== 'number')) {
throw new DeveloperError('Error: Operation is undefined.');
}
//>>includeEnd('debug');

return left > right;
};

Node.prototype._evaluateGreaterThanOrEquals = function(frameState, feature) {
var left = this._left.evaluate(frameState, feature);
var right = this._right.evaluate(frameState, feature);

//>>includeStart('debug', pragmas.debug);
if ((typeof(left) !== 'number') || (typeof(right) !== 'number')) {
throw new DeveloperError('Error: Operation is undefined.');
}
//>>includeEnd('debug');

return left >= right;
};

Expand Down Expand Up @@ -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;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This relies on each types toString function. Should the spec be explicit about how each type is stringified?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

} else if ((typeof(left) === 'number') && (typeof(right) === 'number')) {
return left + right;
}
return left + right;

//>>includeStart('debug', pragmas.debug);
throw new DeveloperError('Error: Operation is undefined.');
//>>includeEnd('debug');

return left + right; // jshint ignore:line
};

Node.prototype._evaluateMinus = function(frameState, feature) {
Expand All @@ -1073,8 +1126,15 @@ define([
return Cartesian3.subtract(left, right, ScratchStorage.getCartesian3());
} else if ((right instanceof Cartesian4) && (left instanceof Cartesian4)) {
return Cartesian4.subtract(left, right, ScratchStorage.getCartesian4());
} else if ((typeof(left) === 'number') && (typeof(right) === 'number')) {
return left - right;
}
return left - right;

//>>includeStart('debug', pragmas.debug);
throw new DeveloperError('Error: Operation is undefined.');
//>>includeEnd('debug');

return left - right; // jshint ignore:line
};

Node.prototype._evaluateTimes = function(frameState, feature) {
Expand All @@ -1098,8 +1158,15 @@ define([
return Cartesian4.multiplyByScalar(right, left, ScratchStorage.getCartesian4());
} else if ((left instanceof Cartesian4) && (typeof(right) === 'number')) {
return Cartesian4.multiplyByScalar(left, right, ScratchStorage.getCartesian4());
} else if ((typeof(left) === 'number') && (typeof(right) === 'number')) {
return left * right;
}
return left * right;

//>>includeStart('debug', pragmas.debug);
throw new DeveloperError('Error: Operation is undefined.');
//>>includeEnd('debug');

return left * right; // jshint ignore:line
};

Node.prototype._evaluateDivide = function(frameState, feature) {
Expand All @@ -1117,8 +1184,15 @@ define([
return Cartesian4.divideComponents(left, right, ScratchStorage.getCartesian4());
} else if ((left instanceof Cartesian4) && (typeof(right) === 'number')) {
return Cartesian4.divideByScalar(left, right, ScratchStorage.getCartesian4());
} else if ((typeof(left) === 'number') && (typeof(right) === 'number')) {
return left / right;
}
return left / right;

//>>includeStart('debug', pragmas.debug);
throw new DeveloperError('Error: Operation is undefined.');
//>>includeEnd('debug');

return left / right; // jshint ignore:line
};

Node.prototype._evaluateMod = function(frameState, feature) {
Expand All @@ -1130,8 +1204,15 @@ define([
return Cartesian3.fromElements(left.x % right.x, left.y % right.y, left.z % right.z, ScratchStorage.getCartesian3());
} else if ((right instanceof Cartesian4) && (left instanceof Cartesian4)) {
return Cartesian4.fromElements(left.x % right.x, left.y % right.y, left.z % right.z, left.w % right.w, ScratchStorage.getCartesian4());
} else if ((typeof(left) === 'number') && (typeof(right) === 'number')) {
return left % right;
}
return left % right;

//>>includeStart('debug', pragmas.debug);
throw new DeveloperError('Error: Operation is undefined.');
//>>includeEnd('debug');

return left % right; // jshint ignore:line
};

Node.prototype._evaluateEqualsStrict = function(frameState, feature) {
Expand Down
13 changes: 13 additions & 0 deletions Source/Scene/PointCloud3DTileContent.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1108,6 +1108,19 @@ define([
fragmentShaderSource : pickFS,
attributeLocations : attributeLocations
});

try {
// Check if the shader compiles correctly. If not there is likely a syntax error with the style.
drawCommand.shaderProgram._bind();
} catch (error) {
//>>includeStart('debug', pragmas.debug);
// Turn the RuntimeError into a DeveloperError and rephrase it.
throw new DeveloperError('Error generating style shader: this may be caused by a type mismatch, index out-of-bounds, or other syntax error.');
//>>includeEnd('debug');

// In release silently ignore and recreate the shader without a style. Tell jsHint to ignore this line.
createShaders(content, frameState, undefined); // jshint ignore:line
}
}

/**
Expand Down