Skip to content
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

set further near plane to fix depth precision for deckgl #8502

Merged
merged 5 commits into from
Aug 14, 2019
Merged

Conversation

ansis
Copy link
Contributor

@ansis ansis commented Jul 17, 2019

fix #7573

This fixes precision issues in custom layers that use deckgl. While the old near plane worked well for our fill-extrusion layers it was causing issues in deckgl layers that do more complex projection in the shaders. This moves the near plane further away to a distance that seems to fix both the z-fighting in deckgl while not creating camera-close-to-building clipping issues.

deckgl would have to change 1 / deck.height to 1 / 50 here to match our change:
https://github.com/uber/deck.gl/blob/bddc36902a2535fa9430a1b450944f83cf4d9d11/modules/mapbox/src/deck-utils.js#L118

@Pessimistress can you confirm this would work for you?

Testing

The debug-z-fighting-deckgl branch contains a debug page that can be used to test this. In the branch I've both patched deckgl and mapbox-gl-js. The third map pane shows both a deckgl layer and a fill-extrusion layer coexisting. To start the example: yarn run start-debug and then open http://localhost:9966/debug/

Launch Checklist

  • briefly describe the changes in this PR
  • write tests for all new functionality
  • manually test the debug page

@ansis ansis requested a review from mourner July 17, 2019 17:04
@chloekraw
Copy link
Contributor

@Pessimistress
Copy link

@ansis Thank you!
For backward compatibility, is there a way we can get the version of mapbox-gl from the Map instance?

@ansis
Copy link
Contributor Author

ansis commented Jul 17, 2019

For backward compatibility, is there a way we can get the version of mapbox-gl from the Map instance?

The version isn't on the map instance but it is exported by the library (mapboxgl.version). Does that work?

// Smaller values worked well for mapbox-gl-js but deckgl was encountering precision issues
// when rendering it's layers using custom layers. This value was experimentally chosen and
// seems to solve z-fighting issues in deckgl while not clipping buildings too close to the camera.
const nearZ = this.height / 50;
Copy link
Contributor

Choose a reason for hiding this comment

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

LGTM
On native side, near set to 0.1 * state.getCameraToCenterDistance() evaluates to height / 90.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there an existing render test that exercises this with extrusion layers?

@Pessimistress
Copy link

@ansis We need to account for both the script tag and node_modules. So we end up having to do something like

function getMapboxVersion() {
  if (typeof mapboxgl === 'undefined') {
    return mapboxgl.version;
  }
  try {
    return require('mapbox-gl').version;
  } catch (err) {
    return 'unknown';
  }
}

And also change bundler configs to avoid bundling mapbox-gl with deck.

@Pessimistress
Copy link

@ansis Just brainstorming, can layer.render receive a third argument that are miscellaneous parameters? Then we can do something like

render(gl, matrix, {near, far}) {
  ...
}

@chloekraw chloekraw added this to the release-queso milestone Aug 12, 2019
switch to round because the projected value was right on the border of
an integer. The near plane difference moved the point 1.9042545318370685e-11
pixels which isn't significant but was breaking this test.
@ansis
Copy link
Contributor Author

ansis commented Aug 13, 2019

I've added the version to the map instance. Exposing the actual parameters is a good idea but in the interest of just getting this merged I've added the version.

Copy link
Contributor

@asheemmamoowala asheemmamoowala left a comment

Choose a reason for hiding this comment

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

@ansis I'm not clear on why the Map object needs to expose version. This is a property of the bundle, and correctly exported on the mapboxgl object, as it applies to more than just the Map class.

// Smaller values worked well for mapbox-gl-js but deckgl was encountering precision issues
// when rendering it's layers using custom layers. This value was experimentally chosen and
// seems to solve z-fighting issues in deckgl while not clipping buildings too close to the camera.
const nearZ = this.height / 50;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there an existing render test that exercises this with extrusion layers?

@asheemmamoowala
Copy link
Contributor

We need to account for both the script tag and node_modules.

@Pessimistress can you say more about what the problem with your current approach is? It looks like you are trying to support multiple ways of importing mapboxgl in your project (or it is demand loaded?)

When a CustomLayerInterface is added to a map, and onAdd is invoked, you should be able to capture the mapboxgl.version and use it to determine how the layer behaves.

@Pessimistress
Copy link

Pessimistress commented Aug 14, 2019

@asheemmamoowala Our custom layer class is published to npm and it does not depend on mapbox-gl. This is clearly allowed as long as it implements the custom layer interface.

From the layer's perspective, we cannot tell how the user is including mapbox. If they are using a bundler, then there will not be a global mapboxgl object. Conditionally requiring an external module is dangerous, because

  • It requires additional bundler setup for users who wish to include mapbox using the script tag
  • It breaks the esm build by mixing require and export
  • It generally violates our coding policy by requiring something that is not in the dependencies list

@ansis
Copy link
Contributor Author

ansis commented Aug 14, 2019

Is there an existing render test that exercises this with extrusion layers?

There are tests with extrusions but none that exercise this because all the values work well for us in mapbox-gl. It's only in deckgl that the issues arise.

When a CustomLayerInterface is added to a map, and onAdd is invoked, you should be able to capture the mapboxgl.version and use it to determine how the layer behaves.

onAdd provides the map, not mapboxgl which is what is motivating adding it to the map

Copy link
Contributor

@asheemmamoowala asheemmamoowala left a comment

Choose a reason for hiding this comment

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

@Pessimistress Thanks for the quick feedback. In that case, adding Map#version seems like a simple enough addition.

LGTM!

@ansis ansis merged commit 5cf6e5f into master Aug 14, 2019
@ansis ansis deleted the fix-7573-deckgl branch August 14, 2019 19:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Set better near/far planes
6 participants