-
Notifications
You must be signed in to change notification settings - Fork 10
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 colormap scaler and offset uniforms #60
Conversation
src/webgl/color/colormap.js
Outdated
|
||
if (!imageColormap) { | ||
return; | ||
} | ||
|
||
return { | ||
u_colormap_texture: imageColormap, | ||
colormapScaler: typeof colormapScaler != 'undefined' ? colormapScaler : 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.
Any reason why not to do colormapScaler || 0.5
? That's easier to read. Are there instances when colormapScaler
is truthy but not set by the user?
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.
colormapScaler || 0.5
is ok, but colormapOffset || 0.5
not, I just wanted to use the same pattern for both
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.
You mean because colormapOffset
can be 0
? Then use
colormapScaler: Number.isFinite(colormapScaler) ? colormapScaler : 0.5
?
That seems easier to follow than typeof colormapScaler != 'undefined'
because you're testing what it should be rather than what it shouldn't be. I.e. if colormapScaler
were a string, it would pass above but crash the shaders probably?
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.
Right, makes sense. Updated.
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.
Alternatively, default values could be changed to scaler = 1, offset = 0, and then ||
is enough, but that would require changes in examples as well.
Yeah I think this is the core issue to be resolved at some point, but it doesn't have to be right now |
d628f43
to
57e28ba
Compare
57e28ba
to
3ee80fd
Compare
Usecase: to be able to use colormap on values [0..1] (scaler = 1, offset = 0; these values might even be default)
linearRescale could be used for this usecase before colormap. However, a module can't be used multiple times in the same shader (#39). In my case (#58), I need either a duplicate linearRescale2 module, or configurable scaler/offset uniforms in colormap.