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

Render fill-extrusion layers #8431

Merged

lbud merged 87 commits into master from extrusions

Apr 27, 2017

Contributor

lbud commented Mar 15, 2017 •

edited

Loading

Opening this for tracking purposes as it's getting close enough to have a public living to-do list:

mention-bot commented Mar 15, 2017

@lbud, thanks for your PR! By analyzing this pull request, we identified @jfirebaugh, @1ec5 and @ansis to be potential reviewers.

lbud mentioned this pull request

[not ready] fill-extrusions tweaks for mapbox-gl-native mapbox/mapbox-gl-js#4433

Merged

1ec5 reviewed

View reviewed changes

Contributor

1ec5 left a comment

Really looking forward to this feature! Thanks for all the hard work you've put into it both here and in GL JS.

I know it's pretty far down on the priority list, but I've left some premature comments about the iOS and macOS side of things, primarily regarding documentation. Take your time with that; the to-do items you've identified are more interesting anyways.

In addition, in order for the iOS and macOS SDKs to build, MGLFillExtrusionStyleLayer.h needs to be added to Mapbox.h, ios.xcodeproj, and macos.xcodeproj. See "Making a type or constant public" and "Adding a source code file" in this guide for details. Let me know if you have any questions.

painter.cp Outdated

		@@ -0,0 +1,421 @@
		#include <mbgl/renderer/painter.hpp>

Contributor

1ec5 Mar 16, 2017

It looks like this file was committed by accident.

platform/darwin/src/MGLFillExtrusionStyleLayer.h

+              NS_ASSUME_NONNULL_BEGIN
+              /**
+               Controls the translation reference point.

Contributor

1ec5 Mar 16, 2017

This change might be worth making upstream:

Controls the translation reference point of a fill extrusion style layer.

Contributor Author

lbud Apr 19, 2017

@1ec5 it might be — this language is copied from *-translate-anchor in every other layer type. I'll ticket this in that repo as a follow-up.

platform/darwin/src/MGLFillExtrusionStyleLayer.h Outdated

+              @property (nonatomic, null_resettable) MGLStyleValue<NSNumber *> *fillExtrusionOpacity;
+              /**
+               Name of image in sprite to use for drawing images on extruded fills. For

Contributor

1ec5 Mar 16, 2017

Since there's no public concept of a spritesheet in the native SDKs, we override any documentation that says "sprite" to say "style image" instead in platform/darwin/scripts/style-spec-overrides-v8.json while we await mapbox/mapbox-gl-js#4086.

platform/darwin/src/MGLFillExtrusionStyleLayer.h Outdated

+              #if TARGET_OS_IPHONE
+              /**
+               The geometry's offset. Values are [x, y] where negatives indicate left and up

Contributor

1ec5 Mar 16, 2017

We also use style-spec-overrides-v8.json to remove any sentence that talks about offsets or padding as arrays, because that isn't how they're exposed by the iOS and macOS SDKs. (They're CGVectors or NSEdgeInsets or UIEdgeInsets, not NSArrays.)

platform/darwin/src/MGLFillExtrusionStyleLayer.h Outdated

+              /**
+               The base color of the extruded fill. The extrusion's surfaces will be shaded
+               differently based on this color in combination with the root `light` settings.
+               If this color is specified as `rgba` with an alpha component, the alpha

Contributor

1ec5 Mar 16, 2017

Per #6107 (comment), we override documentation to remove the phrase "as rgba, because that's CSS syntax, whereas alpha may be specified with -colorWithHue:saturation:brightness:alpha: or -colorWithWhite:alpha: instead of -colorWithRed:green:blue:alpha:.

platform/darwin/src/MGLFillExtrusionStyleLayer.h Outdated

+                 * `MGLInterpolationModeExponential`
+                 * `MGLInterpolationModeInterval`
+               */
+              @property (nonatomic, null_resettable) MGLStyleValue<NSValue *> *fillExtrusionTranslate;

Contributor

1ec5 Mar 16, 2017

Per #6098 (comment), we turn "translate" to "translation" using platform/darwin/scripts/cocoa-conventions-v8.json to prevent a call to the property's getter from looking like a call to an action method.

platform/darwin/src/MGLFillExtrusionStyleLayer.h Outdated

+               */
+              @interface NSValue (MGLFillExtrusionStyleLayerAdditions)
+              #pragma mark Working with FillExtrusion Style Layer Attribute Values

Contributor

1ec5 Mar 16, 2017

s/FillExtrusion/Fill Extrusion/

platform/darwin/src/MGLFillExtrusionStyleLayer.h Outdated

+              #if TARGET_OS_IPHONE
+              /**
+               The base color of the extruded fill. The extrusion's surfaces will be shaded
+               differently based on this color in combination with the root `light` settings.

Contributor

1ec5 Mar 16, 2017

light will in all likelihood be a property of MGLStyle; the word "root" is meaningless in the context of these SDKs.

platform/darwin/src/MGLFillExtrusionStyleLayer.h Outdated

+               ### Example
+               ```swift
+               ```

Contributor

1ec5 Mar 16, 2017

Eventually we'll want to fill in some example code here, via MGLDocumentationTests.swift, but that can be tail work.

platform/darwin/src/MGLFillExtrusionStyleLayer.h Outdated

+              /**
+               An extruded (3D) polygon.
+               You can access an existing fill-extrusion style layer using the

Contributor

1ec5 Mar 16, 2017

s/fill-extrusion/fill extrusion/

Contributor Author

lbud Mar 27, 2017

@1ec5 I am of the opinion (though not strongly) that this should remain hyphenated, I think, for clarity — do you have a strong feeling that it should be fill extrusion?

Contributor

1ec5 Mar 28, 2017

Yes, as a noun phrase, it should be unhyphenated in prose. ("Fill-extrusion style layer" would be more natural as shorthand for "fill and extrusion style layer", like "zoom-property function" as shorthand for "zoom-and-property function".) Unlike in JavaScript, the hyphenated form fill-extrusion will never appear in the developer's code.

Contributor

anandthakker commented Mar 16, 2017

I suspect there may be a bug in the way we're handling zoom-and-property functions — investigate

FYI, our handling of these underwent some recent changes (by @mollymerp and I) - happy to help 💭 / 🕵️

Contributor Author

lbud commented Mar 16, 2017

Thanks for the 👀 @1ec5! All platform-specific code committed thus far has been autogenerated with scripts/make targets so your attention is especially appreciated 🙇‍♀️ — adding to the list.

lbud force-pushed the extrusions branch from 6bcd775 to f9c6cec Compare

March 20, 2017 23:04

1ec5 added this to the ios-v3.6.0 milestone

1ec5 added Core feature GL JS parity Google Maps parity MapKit parity labels

1ec5 mentioned this pull request

3D extrusion #6745

Closed

lbud force-pushed the extrusions branch from d682d56 to 3661090 Compare

March 27, 2017 21:03

1ec5 reviewed

View reviewed changes

Contributor

1ec5 left a comment

Remember to rerun make darwin-style-code to incorporate the changes in d682d56.

platform/darwin/scripts/style-spec-overrides-v8.json

+                    "doc": "The geometry's offset."
+                  },
+                  "fill-extrusion-color": {
+                    "doc": "The base color of this layer. The extrusion's surfaces will be shaded differently based on this color in combination with the `light` settings. If this color is specified with an alpha component, the alpha component will be ignored; use `fill-extrusion-opacity` to set layer opacityco."

Contributor

1ec5 Mar 28, 2017

Typo: "opacityco".

platform/darwin/scripts/style-spec-overrides-v8.json

@@ @@ -50,6 +53,17 @@ @@
                     "doc": "Name of image in style images to use for drawing image fills. For seamless patterns, image width and height must be a factor of two (2, 4, 8, ..., 512)."
                   }
                 },
+                "paint_fill-extrusion": {
+                  "fill-extrusion-pattern": {
+                    "doc": "Name of image in style images to use for drawing image fill-extrusions. For seamless patterns, image width and height must be a factor of two (2, 4, 8, ..., 512)."

Contributor

1ec5 Mar 28, 2017

"Image fills" sort of made sense for fill-pattern, but I'm not sure about "image fill-extrusions". How about "for drawing the surfaces of the extrusion"?

This was referenced Mar 29, 2017

Clamp negative heights to zero in extrusion shaders mapbox/mapbox-gl-js#4518

Merged

[not ready] Un-ignore fill-extrusion tests on native mapbox/mapbox-gl-js#4519

Merged

lbud force-pushed the extrusions branch 5 times, most recently from 4a68bc9 to 76a3fb1 Compare

April 6, 2017 18:36

Contributor Author

lbud commented Apr 7, 2017

@jfirebaugh @kkaefer I'm trying to figure out one last thing here: there's a failing TileCover test on a few Bitrise platforms (though passing on Travis) — the returned tiles the same but are slightly out of order; this is the result of the changes in transform_state.cpp to scale the projection matrix vertically to meters per pixel (corresponding gl-js change: mapbox/mapbox-gl-js#3509).

Aside from that however this is ready for review (so please start reviewing whenever you feel ready) — the only thing that may change is that small part of transform_state.cpp. I'm not sure if this would be easier to review if I squashed or rearranged some commits, or if this is reviewable as-is — if I can make it easier for you let me know 🙏

lbud requested review from kkaefer, jfirebaugh, 1ec5 and ivovandongen

April 7, 2017 18:35

Contributor Author

lbud commented Apr 7, 2017 •

edited

Loading

Also tagging in @ivovandongen @1ec5 to do a pass over generated Darwin/Android code 🙏

Lauren Budorick added 3 commits

April 26, 2017 16:52


          Fix for previous commit: calculate a projection matrix just for the f…

72278bf

…ill-extrusion layer with a further near clip plane, and leave the global projection matrix alone


          Simplify OffscreenTexture: use DepthComponent (as defined by preproce…

086fa8d

…ssor conditional in types.hpp) in all cases


          Rebase conflict resolution

764576f

lbud force-pushed the extrusions branch from 40104e7 to 764576f Compare

April 27, 2017 00:53


          ? redundant style.hpp in cmake

4a749c0

Contributor Author

lbud commented Apr 27, 2017

Recording the solution to the depth buffer issue for posterity's sake:
In an ideal world we'd be able to use 24-bit depth buffers everywhere to solve the artifacts issue, but in reality we can't count on anything better than GL ES 2.0 without extensions. The approach I'm going with here is compatible with core ES2 (16-bit depth buffers — removes all references to 24-bit depth buffers, even where supported): for the projection matrix used for fill-extrusions, set the near clip plane to 100 (a sort of arbitrary number that seems to suit our needs well). This way we don't waste a ton of unnecessary depth precision on very near space where we aren't actually rendering anything, and we have enough precision for the rest of the scene.

Contributor

ivovandongen commented Apr 27, 2017

@lbud I get a failed UI test on FillExtrusionLayerTest#testSourceLayer:

org.junit.ComparisonFailure: expected:<[]> but was:<[composite]>
at org.junit.Assert.assertEquals(Assert.java:115)
at org.junit.Assert.assertEquals(Assert.java:144)
at com.mapbox.mapboxsdk.testapp.style.FillExtrusionLayerTest.testSourceLayer(FillExtrusionLayerTest.java:90)
...

Lauren Budorick and others added 2 commits

April 27, 2017 10:52


          Store both normal tile matrix + near-clipped matrix in RenderTiles to…

b22e8aa

… avoid setting and re-setting matrix


          [android] - add accessor support for source layer

695da93

Member

tobrun commented Apr 27, 2017

@lbud 695da93 resolves the failing test.

Lauren Budorick and others added 3 commits

April 27, 2017 13:34


          Update SHA after merging mapbox/mapbox-gl-js#4519

bf52d28


          Update gl-js SHA

30df15b


          Update gl-js SHA

b450c4c

jfirebaugh mentioned this pull request

Skip image tests for native mapbox/mapbox-gl-js#4653

Merged

lbud merged commit f6e79d7 into master

lbud deleted the extrusions branch

April 27, 2017 22:56

This was referenced Apr 27, 2017

Add light property to MGLStyle #8840

Closed

Add light settings to MapboxMap #8841

Closed

Contributor

1ec5 commented Apr 27, 2017

platform-specific getLight/setLight APIs can be added in follow-up PRs 🙏 😄

Android: #8841
iOS and macOS: #8840

tobrun mentioned this pull request

Release Android v5.1.0-beta1 #8856

Closed

12 tasks

1ec5 mentioned this pull request

[ios, macos] Light property implementation in MGLStyle #9043

Merged

4 tasks

This was referenced May 18, 2017

Release Android v5.1.0-beta2 #8975

Closed

Release Android v5.1.0-beta.3 #9114

Closed

tobrun mentioned this pull request

Release Android v5.1.0-beta.4 #9231

Closed

12 tasks

1ec5 mentioned this pull request

Update SMCalloutView to fix iOS 10 build warnings (redux) #9314

Merged

Contributor

1ec5 commented Jun 21, 2017 •

edited

Loading

Looks like this PR rolled back #6619 (nfarina/calloutview#107): #9314.

tobrun mentioned this pull request

Release Android v5.1.0-beta.5 #9315

Closed

11 tasks

tobrun mentioned this pull request

Release Android v5.1.0 #9404

Closed

16 tasks

lbud mentioned this pull request

Small-radius heatmap flickers when moving the map mapbox/mapbox-gl-js#5563

Open

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Core feature GL JS parity Google Maps parity MapKit parity Tangram parity