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

Set color conditional #3655

Merged
merged 9 commits into from
Mar 2, 2016
Merged

Set color conditional #3655

merged 9 commits into from
Mar 2, 2016

Conversation

ggetz
Copy link
Contributor

@ggetz ggetz commented Mar 1, 2016

Added support for color conditionals. They are in the form:

color : {
   conditional : {
      'booleanExpression1` : `colorExpression1',
      'booleanExpression2` : `colorExpression2',
     // etc...
   },
   default : `defaultColorExpression`
}

We check each condition in order, and, if true, evaluate the corresponding color expression.

I added the file ColorConditional.js for the conditionals. It has both an evaluate(feature) and evaluate(feature, result)` function. I think we may only need the latter?

I deleted ColorMapExpression and ColorRampExpression.

I added tests for ColorConditional and for Cesium3DTileStyle and Cesium3DTileset to check if the style is properly applied.

I also updated the Sandcastle example.

I'm having trouble with the Change Style Using ID and Change Style Using Height buttons, because they generate the conditionals depending on the number of colors, and I was having trouble making the conditional JSON expression, because the Boolean expressions have to be strings: (ex. '${height} < 1' : 'color("red")' not ${height < 1 : 'color("red")'). So that part of the Sandcastle example has not yet been updated.

@pjcozzi
Copy link
Contributor

pjcozzi commented Mar 1, 2016

I think we may only need the latter?

I think we need both. We should probably position this as a general "conditional", not specific to colors even if that is where the original use case came from.

@ggetz
Copy link
Contributor Author

ggetz commented Mar 1, 2016

I think we need both. We should probably position this as a general "conditional", not specific to colors even if that is where the original use case came from.

OK, that should be pretty easy, Should I do that in this PR?

@pjcozzi
Copy link
Contributor

pjcozzi commented Mar 1, 2016

Should I do that in this PR?

Yes, let me know if you run into any ideas. I will also do the spec/schema for this tomorrow or Friday.

@pjcozzi
Copy link
Contributor

pjcozzi commented Mar 1, 2016

I was having trouble making the conditional JSON expression, because the Boolean expressions have to be strings

What exactly is the problem? Can't we just generate strings?

@ggetz
Copy link
Contributor Author

ggetz commented Mar 1, 2016

I set up the conditional like this:

var conditional  = {};

Then set up the conditional and color expression strings and use

conditional[conditionalExpression] = colorExpression;

but this outputs { ${height} < 1 : 'color("red")' } not { '${height < 1' : 'color("red")' }. Is there a way to make the "attribute" a string?

setRuntimeDefault(this);
}

defineProperties(ColorConditional.prototype, {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would actually remove this for now until we expose the AST (probably not for the first pull request into 3d-tiles).

Otherwise, this looks like a fast get/set, but it is just as bad as assigning a new style since it forces a recompile.

@pjcozzi
Copy link
Contributor

pjcozzi commented Mar 1, 2016

Looks good, just those comments.

@pjcozzi
Copy link
Contributor

pjcozzi commented Mar 1, 2016

but this outputs { ${height} < 1 : 'color("red")' } not { '${height < 1' : 'color("red")' }. Is there a way to make the "attribute" a string?

I still don't understand; is this with JSON.stringify? The conditional is a string containing strings. Is there an issue escaping them?

@ggetz
Copy link
Contributor Author

ggetz commented Mar 1, 2016

I still don't understand; is this with JSON.stringify? The conditional is a string containing strings. Is there an issue escaping them?

I didn't use JSON.stringify- I will try it with that.

@ggetz
Copy link
Contributor Author

ggetz commented Mar 1, 2016

I think we need both. We should probably position this as a general "conditional", not specific to colors even if that is where the original use case came from.

The default default value is currently the expression color("#ffffff"). Should we not have a default default value since we don't know what type this is supposed to evaluate to since it's being used in a general sense?

@pjcozzi
Copy link
Contributor

pjcozzi commented Mar 1, 2016

Let's remove default from the schema completely since a conditional that is just 'true' at the end is the same thing.

@ggetz
Copy link
Contributor Author

ggetz commented Mar 2, 2016

In the case that someone doesn't provide a 'true' condition, it will evaluate the expressions for any features that don't fall into the other conditionals as undefined. Is that behavior we want?

@pjcozzi
Copy link
Contributor

pjcozzi commented Mar 2, 2016

Is that behavior we want?

Yes.

@ggetz
Copy link
Contributor Author

ggetz commented Mar 2, 2016

OK, I updated the name of the file to ExpressionConditional instead of ColorConditional, removed the default property, updated the tests, and updated the sandcastle example.

@pjcozzi
Copy link
Contributor

pjcozzi commented Mar 2, 2016

Looks great, just one rename request: throughout, rename ExpressionConditional to ConditionalExpression.

@ggetz
Copy link
Contributor Author

ggetz commented Mar 2, 2016

Updated.

@pjcozzi
Copy link
Contributor

pjcozzi commented Mar 2, 2016

👍

pjcozzi added a commit that referenced this pull request Mar 2, 2016
@pjcozzi pjcozzi merged commit c00c531 into CesiumGS:3d-tiles-style Mar 2, 2016
@pjcozzi pjcozzi deleted the set-color-conditional branch March 2, 2016 18:46
@pjcozzi pjcozzi mentioned this pull request Mar 3, 2016
85 tasks
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.

None yet

2 participants