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

[core] Refactor wireframe to match JS overdraw #5403

Merged
merged 6 commits into from
Jun 20, 2016

Conversation

brunoabinader
Copy link
Member

This PR:

  • Adds a new BlendColor GL configuration value
  • Generates an additional overdraw fragment shader & program for each shader type
  • Refactors the wireframe mode to behave like JS overdraw mode, where each render pass over the same place increases the color by 1/8 from black to white.

Fixes #5371.

Snapshot:
screen shot 2016-06-18 at 7 33 39 pm

Notes:

  • Although similar, it seems that Wireframe and Overdraw serves different purposes - we could add a new debug mode instead of replacing wireframe implementation.
  • Do we really need to overdraw SDFs?

👀 @mapbox/gl

@brunoabinader brunoabinader added GL JS parity For feature parity with Mapbox GL JS Core The cross-platform C++ core, aka mbgl labels Jun 18, 2016
@brunoabinader brunoabinader force-pushed the brunoabinader-overdraw branch 2 times, most recently from 9979db1 to 209f8ed Compare June 18, 2016 17:21
@brunoabinader
Copy link
Member Author

I suspect the debug/overdraw render test differs between -js and -native because in -native we optimize the background layer via glClear. However, I'm yet to find a proper solution for that.

@brunoabinader brunoabinader added the ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold label Jun 18, 2016
@brunoabinader
Copy link
Member Author

When 1) disabling the glClear optimization for the bottommost solid background layer and 2) not resetting the blend function in the translucent render pass we are now getting 1:1 overdraw results between -js and -native 🎉:
screen shot 2016-06-19 at 6 19 12 pm

@kkaefer
Copy link
Contributor

kkaefer commented Jun 20, 2016

Awesome!

@brunoabinader brunoabinader merged commit 6c9e00b into master Jun 20, 2016
@brunoabinader brunoabinader deleted the brunoabinader-overdraw branch June 20, 2016 10:41
@friedbunny friedbunny added the iOS Mapbox Maps SDK for iOS label Jun 20, 2016
@friedbunny friedbunny modified the milestone: ios-v3.3.0 Jun 20, 2016
@friedbunny
Copy link
Contributor

I initially thought this would be good to cherry-pick into the release-ios-v3.3.0 branch, but it includes too many intervening core changes for that to be tenable.

wireframe effect. */
MGLMapDebugWireframesMask = 1 << 5,
/** Overdraw inspector. */
MGLMapDebugOverdrawsMask = 1 << 5,
Copy link
Contributor

Choose a reason for hiding this comment

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

This enum value needs a more descriptive name, and the comment is somewhat nonsensical to an iOS developer.

@1ec5
Copy link
Contributor

1ec5 commented Jun 20, 2016

@friedbunny, @brunoabinader renamed the public enum value, so we either need to revert the Objective-C changes or backport them to the release branch. Otherwise, the release after 3.3.0 will break backwards compatibility.

@friedbunny
Copy link
Contributor

@1ec5 I suppose just porting the Obj-C changes without including the refactored implementation itself is a thing we can do. Functionality would change after v3.3.0, but the API would remain the same.

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 GL JS parity For feature parity with Mapbox GL JS iOS Mapbox Maps SDK for iOS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants