-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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 fill-extrusion-vertical-gradient property #6841
Conversation
Is @mapbox/studio ok with including this? Seems like this is a desired option. |
In general we're a little concerned about the spec getting bloated, but we're good with this addition; it'll be easy enough for us to support |
955845a
to
096d885
Compare
1cb0392
to
d13dada
Compare
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.
looks good pending a few minor comments/changes. ποΈ
@@ -24,7 +24,8 @@ export type FillExtrusionUniformsType = {| | |||
'u_matrix': UniformMatrix4f, | |||
'u_lightpos': Uniform3f, | |||
'u_lightintensity': Uniform1f, | |||
'u_lightcolor': Uniform3f | |||
'u_lightcolor': Uniform3f, | |||
'u_vertical_gradient': Uniform1f |
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.
this is just 1 or 0 right? in that case we can use an integer uniform Uniform1i
to follow the convention of other uniforms used as booleans such as u_is_zoom_constant
for symbol icons/sdfs
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.
This actually needs to be a float because the calculation in the shader has float constants.
@@ -80,7 +84,8 @@ const extrusionTextureUniforms = (context: Context, locations: UniformLocations) | |||
|
|||
const fillExtrusionUniformValues = ( | |||
matrix: Float32Array, | |||
painter: Painter | |||
painter: Painter, | |||
verticalGradient: number |
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 clarity, I lean towards passing this value to the *UniformValues
functions as a boolean, and casting only as the value is bound to its uniform location.
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.
Uniform1i
requires a number input so casting the boolean to a number only when binding to the location causes a Flow error. Is there a way around that?
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.
yep you use the +
like here https://github.com/mapbox/mapbox-gl-js/blob/master/src/render/program/symbol_program.js#L116
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.
Ah yeah, I was mistakenly casting to a number elsewhere in the program, causing a flow error. This is fixed now.
@@ -47,7 +48,11 @@ void main() { | |||
|
|||
// Add gradient along z axis of side surfaces | |||
if (normal.y != 0.0) { | |||
directional *= clamp((t + base) * pow(height / 150.0, 0.5), mix(0.7, 0.98, 1.0 - u_lightintensity), 1.0); | |||
// This avoids another branching statement, but multiplies by a constant of 0.84 if no vertical gradient, |
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.
do we have any idea where the magic -0.84 comes from? maybe nicki would know
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.
why would Nicki know? i have no idea where it comes from
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.
Nicki worked on the extrusion shaders w lbud
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.
@nickidlugash do you remember where this magic number came from?
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.
@ryanhamley @mollymerp I didn't work with lbud on this PR at all, but having worked on the initial implementation of this shader, my guess is that 0.84 is the lower clamp in the gradient logic when u_lightintensity
is the default value of 0.5 (mix(0.7, 0.98, 0.5)
= 0.84). I assume it's negative so that when u_vertical_gradient
is 0, that first line just evaluates to 0.84.
0.7 and 0.98 are magic numbers that I came up with to keep the vertical gradient consistently subtle, regardless of how intense the light is.
I actually wonder though if it would be better not to multiply by a constant here if there is no vertical gradient (i.e. something like (1.0 - u_vertical_gradient)
instead of the current top line). Multiplying by a constant seems like it would unnecessarily limit the range of light intensity such that you could never get as intense a light when intensity=1.
@lbud, @ryanhamley, @mollymerp, @nickidlugash Thanks so much for your awesome work on this. My team is looking forward to getting this into our project right away. |
This PR adds a new boolean data-constant paint property,
fill-extrusion-vertical-gradient
( π€ a bit verbose β I'm open to better ideas). It defaults totrue
(the current behavior), but if set tofalse
, does not apply a vertical gradient along the sides of fill-extrusion features.Closes #5768.
I only ran benchmarks for
LayerFillExtrusion
since nothing else should be affected, and it shows no real change: https://bl.ocks.org/lbud/raw/d353b63e34c7fcb6e4ec2698370fd6b9/Launch Checklist
@mapbox/maps-design
if this PR includes style spec changes