-
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-extrude-height and fill-extrude-base properties. #3223
Conversation
Is this for real? 😮 Can you post some screenshots of this? |
@@ -43,6 +43,7 @@ toc: | |||
- CameraOptions | |||
- AnimationOptions | |||
- StyleOptions | |||
- LightingOptions |
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.
Shall we rename this to LightOptions
like in mapbox/mapbox-gl-style-spec#495 (comment)?
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 removed Lighting/LightOptions
entirely since now as a part of style
, it is more clearly derived from the spec.
group.elementArray.emplaceBack(indices[triangleIndices[j]], | ||
indices[triangleIndices[j + 1]], | ||
indices[triangleIndices[j + 2]]); | ||
} |
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.
Did you consider creating a separate FillExtrusionBucket
class rather than branching on this.fillType === 'fill'
within methods of FillBucket
?
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.
done
gl.clearStencil(0x80); | ||
gl.stencilMask(0xFF); | ||
gl.clear(gl.COLOR_BUFFER_BIT | gl.DEPTH_BUFFER_BIT); | ||
gl.stencilMask(0x00); |
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.
Does this overwrite our tile clipping masks?
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.
Here's what I've figured out:
gl.clear
only affects the currently bound framebuffer and is needed here- I can delete several of these lines (basically all but the
gl.clear
) and get visually the same results. My best guess is that this was in here because the original implementation (from which I copied these) rendered to the map as a normal layer, so it needed to fiddle with the stencil mask (so as to clear the entire screen). Now that we're rendering in a new framebuffer, it doesn't need to redraw a mask that's not actually there — so, deleting all these lines except thegl.clear
, which clears its last frame draw. (Fwiw/for future reference: based on my testing I actually don't think it was affecting the global stencil mask anyway because of the different framebuffer.) - testing for the clipping masks made me realize I forgot to flip the depthMask back off after this draw, which was causing fills to z-fight if above an extrusion layer — fixed 😅
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.
testing for the clipping masks made me realize I forgot to flip the depthMask back off after this draw, which was causing fills to z-fight if above an extrusion layer — fixed 😅
nope I was totally wrong, this broke other fills and also is #3320
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.
☝️ fixed
for (var key in lightOptions) { | ||
this.light[key] = lightOptions[key]; | ||
} | ||
}; |
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 see Painter
as a collection of GL utilities with no knowledge of map primitives and Style
as the owner of state about the map style. Why did you choose to put this light
state on Painter
rather than Style
?
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.
changed
if (widthTextures) { | ||
var textures = widthTextures[height || width]; | ||
return textures && textures.length > 0 ? textures.pop() : null; | ||
} |
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.
Did you verify that the performance benefits of reusing textures are still worthwhile with these added constraints? Given that we never deallocate these textures, are you concerned about resource leaks?
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.
fixed — we're now saving one texture at a time that can be reused for every subsequent frame until the map is resized, at which point we delete that texture and create a new one for the new size
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.
Perfect 😄
|
||
return this; | ||
} | ||
}); |
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 class could be simplified by taking advantage of the existing StyleDeclaration
and StyleTransition
classes.
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.
done
* @param {LightOptions} options Options describing the target light properties of the transition. | ||
* @param {Object} [eventData] Data to propagate to any event listeners. | ||
* @fires lightstart | ||
* @fires lightend |
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 like the idea of an events that mark the beginning and end of a style property transition. I'm not sure we need such an event specifically for light
properties, however. What do yout think about a follow-up PR that adds transitionstart
and transitionend
events?
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.
Follow-up PR sounds good 👍
this.painter.setLight({ | ||
anchor: anchor | ||
}); | ||
} else throw new Error('light.anchor must be one of: `map`, `viewport`'); |
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.
All validation logic that can be done in the style spec should be done in the style spec.
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.
Because of the setLight
runtime API I think we should validate here, no?
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.
light
is associated with the style.
We keep all style validation code separate from GL JS so that we can have a standalone style validation utility. This is used, for example, by the style upload API.
Here is an example of where we use the style spec's validation code to validate layers
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.
done
throw new Error('light.direction must be an array of three numbers'); | ||
|
||
if (direction[1] < 0 || direction[1] >= 360) util.warnOnce('Azimuthal angle will be wrapped to [0, 360)'); | ||
if (direction[2] < 0 || direction[2] >= 180) util.warnOnce('Polar angle will be clamped to [0, 180)'); |
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.
Hard failures or smart fallbacks are preferable to warnings.
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.
gone
r * Math.sin(azimuthal) * Math.sin(polar), | ||
r * Math.cos(polar) | ||
]; | ||
}, |
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 looks like a good candidate for a standalone util
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.
done
Per mapbox/mapbox-gl-style-spec#240 (comment) I've disabled |
@dagjomar it is for real :) |
…y rendering to the whole screen (when given artificial color frag data)
* Move light into its own UI Map subcomponent * Smoothly transitioning light APIs * Abstract light.direction into spherical coordinates/hide cartesian altogether
8d6415e
to
59a3bcb
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.
This is amazing @lbud! Congratulations! Not only is this code beautiful but it was delivered with positivity and finesse. I'm so proud to have you in the GL JS family.
🍾 time!
And enjoy your vacation! |
This PR adds support for
fill-extrude-height
andfill-extrude-base
.I would love to start getting some 👀 on this branch.
Notably, this diverges from our existing patterns in the following ways:
draw_extrusion
handles complete drawing of extruded fills; this diversion happens in painter.jsTo do:
spherical interpolationCloses #684.
Companion PRs:
mapbox/mapbox-gl-style-spec#495
mapbox/mapbox-gl-test-suite#145
mapbox/mapbox-gl-shaders#25
@jfirebaugh @lucaswoj @mourner @mollymerp
Launch Checklist