-
Notifications
You must be signed in to change notification settings - Fork 75
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
Improve unstepped contours mapping of color and range #1079
Conversation
For stepped data, the range values need to have one more entry than the color values, since the range values define the divisions where colors change. For non-stepped data, we were still asking for one more range entry than color entry, but the final range entry was not used. Allow a one-to-one mapping for non-stepped data, as this is more obvious.
@@ -63,7 +64,8 @@ var meshFeature = require('./meshFeature'); | |||
* @property {number[]} [rangeValues] An array used to map values to the | |||
* `colorRange`. By default, values are spaced linearly. If specified, the | |||
* entries must be increasing weakly monotonic, and there must be one more | |||
* entry then the length of `colorRange`. | |||
* entry then the length of `colorRange` if the contour is stepped, or the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
stepped or unstepped may not be obvious to some, should we also add "continuous" and "discreet"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we just change the documentation, or do you think we should allow multiple names for the parameter, too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for the backward compatibility, adding multiple names for the parameters makes sense to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay -- let's do that in a separate PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sounds good
@@ -25,7 +25,7 @@ void main () { | |||
step = steps - 0.5; | |||
} | |||
} else { | |||
step = valueVar; | |||
step = valueVar + 0.5; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is 0.5 added to the valueVar? I did not look at the complete code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is valueVar range values or values?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I should have documented this better as a bug fix and not directly related to the rest of the PR. Specifically, we have a small texture which is the color map of the colors for the normalized range values. The color of the fragment is interpolated from the texture. Without the + 0.5
, it was picking the wrong color: with + 0.5
an integer value will be centered in texture's pixel coordinate and therefore that exact color. In our examples, this is a relatively subtle shift. You can construct an example where it is more obvious (for instance, a two color range where a lot of the values are at the high and low values would have shown some areas as halfway between the two colors instead of exactly at one of them).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay thanks, that clarifies
For stepped data, the range values need to have one more entry than the color values, since the range values define the divisions where colors change. For non-stepped data, we were still asking for one more range entry than color entry, but the final range entry was not used. Allow a one-to-one mapping for non-stepped data, as this is more obvious.