-
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
Introduce ColorMode, StencilMode, DepthMode abstractions #5826
Conversation
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.
Nit: all those modes look immutable (they're not modified once created), so should we make them constants instead of allocating new objects/arrays on every draw call?
src/gl/stencil_mode.js
Outdated
class StencilMode { | ||
func: CompareFuncType; | ||
ref: number; | ||
mask: 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.
There are two "masks" in the stencil state: the write mask (set by glStencilMask
), and the function mask (set by the third parameter to glStencilFunc
). This definition uses mask
for both of them. The place where I'd expect this to create a problem is stencilModeForClipping
, which should use 0 for the write mask and 0xFF for the function mask (it looks like native uses a dynamic value for the function mask -- not sure if this discrepancy matters).
In native, I added the SimpleTest
and MaskedTest
types to enforce that the function mask is omitted (because irrelevant) when the test function is NEVER
or ALWAYS
. The moral equivalent with flow types is something like:
type StencilTest =
| { func: NEVER, mask: 0 }
| { func: LESS, mask: number }
| { func: EQUAL, mask: number }
| ...
| { func: ALWAYS, mask: 0 };
src/gl/color_mode.js
Outdated
blendColor: ?Color; | ||
mask: ColorMaskType; | ||
|
||
constructor(blendFunction: BlendFuncType, blendColor: ?Color, mask: ColorMaskType) { |
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.
In native, a default-initialized color is rgba(0,0,0,0)
, which corresponds to the default value for glBlendColor
. In GL JS you can use Color.transparent
for the same purpose, and make blendColor
non-nullable.
src/render/draw_background.js
Outdated
|
||
painter.setDepthSublayer(0); | ||
context.setStencilMode(StencilMode.disabled()); | ||
context.setDepthMode(painter.depthModeForSublayer(0, true)); |
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.
In native, RenderBackgroundLayer::render
uses DepthMode::ReadOnly
, which is equivalent to false
. Is this a bug? (The ReadOnly
and ReadWrite
enum values are nice for readability -- can JS do something similar?)
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.
Oh — actually both are wrong, I think. Since we draw opaque backgrounds in the opaque pass we should be writing to the depth mask only on that pass, as drawFill
does. (I happened to try a test case with two opaque background layers, and they're rendered in the wrong order — only the bottom layer appears. Obviously this is an edge case that probably very few people would ever need to support, but 🤷♀️)
Re: ReadOnly
and ReadWrite
enums — 👍
} | ||
|
||
depthModeForSublayer(n: number, mask: DepthMaskType, func: ?DepthFuncType): DepthMode { | ||
const farDepth = 1 - ((1 + this.currentLayer) * this.numSublayers + n) * this.depthEpsilon; |
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 might be a good opportunity to fix mapbox/mapbox-gl-native#9713 (though it's ticketed in -native, we could reverse it here instead).
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 tried doing this and must have been getting something wrong as most things no longer rendered correctly (if at all) — I'm going to merge this as-is and we can take on mapbox/mapbox-gl-native#9713 separately.
src/render/draw_fill_extrusion.js
Outdated
@@ -28,6 +30,7 @@ function draw(painter: Painter, source: SourceCache, layer: FillExtrusionStyleLa | |||
drawExtrusion(painter, source, layer, coords[i]); | |||
} | |||
} else if (painter.renderPass === 'translucent') { | |||
painter.context.setColorMode(painter.colorModeForRenderPass()); |
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 not to group this with the other set*Mode
calls inside drawExtrusionTexture
?
…e, fix background depth mask mode to be pass-specific
Abstracts all depth-, color- and stencil-related state manipulation into "modes," so that each render function is responsible for setting its own modes (with
context.set*Mode
as an intermediate step, but eventually as part ofprogram.draw
). This eliminates all instances of render functions needing to reset assumed state after they're finished 🎉I figured out all of the quirks I mentioned offline, so
context.setStencilMode
,context.setColorMode
, andcontext.setDepthMode
behave almost exactly the same as in native (the one slight difference being how we handleblendColor
, which is set mostly as default-initialized in native, but is optional and often therefore not set here).Benchmarks: https://bl.ocks.org/anonymous/raw/5a607b699c5f57ed705e4a7e2aa342a1/
The
LayerBackground
test does consistently look significantly slower (~1.89 ms, as opposed to previously ~0.59 ms) but I don't feel that's concerning given that most maps use one or very few background layer.I don't feel comfortable merging this yet until I add a bunch of render tests that test various combinations of layers (which would have caught e.g. 5565087 — I'm going to add the render test for that to a new branch off of current master).
Refs #145.
Refs mapbox/mapbox-gl-native#10655.