Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

Fix rendering of fill outlines that have a different color than the fill itself #9699

Merged
merged 1 commit into from
Aug 7, 2017

Conversation

kkaefer
Copy link
Member

@kkaefer kkaefer commented Aug 3, 2017

Fixes #9690.

We're rendering polygon outlines in the same color as the polygon in order to achieve an antialiasing effect. In this case, the rendering order works like this:

  • (opaque pass) draw fill, write to depth buffer
  • (translucent pass) draw outline, obeying depth buffer that cuts out half of the outline, so that we only render one "side" of the line

When the outline color didn't match the fill color, we did it this way:

  • (opaque pass) draw fill, write to depth buffer
  • (translucent pass) draw outline, not obeying depth buffer for this particular layer, but obeying it for other layers

This seems to be the part that broke:

  • We no longer ignore the depth buffer when rendering because the sublayer for a colored outline now has value 2 (instead of 0, as it used to be)
  • We're writing fragments we draw for outlines to the depth buffer, but should not do this, since the outline shader produces translucent fragments
  • We're drawing the outline before drawing the fill when the fill color is translucent

This change restores usage of sublayer 0 for mismatched fill/outline color, and sublayer 2 for identical fill/outline color. It also changes the depth mode to prevent writing to the depth buffer when rendering outlines.

Remaining tasks:

  • Add render tests to the test suite
  • Verify behavior for DDS fill color/outline color

@kkaefer kkaefer added Core The cross-platform C++ core, aka mbgl rendering ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold labels Aug 3, 2017
@kkaefer kkaefer requested a review from jfirebaugh August 3, 2017 10:34
Copy link
Contributor

@jfirebaugh jfirebaugh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Related issues: #8760, #6927, mapbox/mapbox-gl-js#4601.

draw(parameters.programs.fillOutline,
gl::Lines{ 2.0f },
parameters.depthModeForSublayer(
unevaluated.get<FillOutlineColor>().isUndefined() ? 2 : 0,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like this condition is the opposite of gl-js? Am I reading that right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right. However, upon further inspection, we're using different formulas to compute the depth mode:

JS:

const farDepth = 1 - ((1 + this.currentLayer) * this.numSublayers + n) * this.depthEpsilon;
const nearDepth = farDepth - 1 + this.depthRange;

Native:

const float nearDepth = ((1 + currentLayer) * numSublayers + n) * depthEpsilon;
const float farDepth = nearDepth + depthRangeSize;

I'll ticket this as a separate change that we'll have to make.

@kkaefer kkaefer force-pushed the fix-colored-outline-rendering branch 2 times, most recently from b70ba6d to 19a32ec Compare August 4, 2017 15:51
@kkaefer
Copy link
Member Author

kkaefer commented Aug 4, 2017

This change also fixes #8760. It requires mapbox/mapbox-gl-js#5099 to be merged simultaneously.

@kkaefer kkaefer force-pushed the fix-colored-outline-rendering branch from 19a32ec to 8bf13ab Compare August 4, 2017 16:37
@kkaefer kkaefer force-pushed the fix-colored-outline-rendering branch from 8bf13ab to b0fd292 Compare August 7, 2017 08:06
@kkaefer kkaefer merged commit e8bfcd0 into master Aug 7, 2017
@kkaefer kkaefer deleted the fix-colored-outline-rendering branch August 7, 2017 08:41
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Core The cross-platform C++ core, aka mbgl ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold rendering
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants