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

[core] Implement circle-stroke properties#7356

Merged
jfirebaugh merged 2 commits into
masterfrom
circle-stroke
Dec 9, 2016
Merged

[core] Implement circle-stroke properties#7356
jfirebaugh merged 2 commits into
masterfrom
circle-stroke

Conversation

@jfirebaugh
Copy link
Copy Markdown
Contributor

@jfirebaugh jfirebaugh commented Dec 8, 2016

Also includes stubs for fill-extrusion layer, because most of the code was auto-generated.

Submitting this ahead of the data-driven styling PR because the DDS PR also requires updating the style-spec and regenerating the style code, and these changes would otherwise come along for the ride there.

I didn't run the SDK code generation. Since this is strictly additions, I'm hopeful they'll compile nevertheless. Adding SDK bindings for circle-stroke properties can then be done in a separate step.

Fixes #7231.

@mention-bot
Copy link
Copy Markdown

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

Also includes stubs for fill-extrusion layer, because most of the code was auto-generated.
@jfirebaugh
Copy link
Copy Markdown
Contributor Author

jfirebaugh commented Dec 8, 2016

I went ahead and ran the SDK code generation as well. It depends on mapbox/mapbox-gl-style-spec#617.

@jfirebaugh jfirebaugh requested review from 1ec5 and tobrun December 8, 2016 21:19
@1ec5
Copy link
Copy Markdown
Contributor

1ec5 commented Dec 8, 2016

▸ Compiling MGLMapView.mm

❌  /Users/vagrant/git/platform/darwin/src/NSValue+MGLStyleEnumAttributeAdditions.h:10:9: 'MGLFillExtrusionStyleLayer.h' file not found

#import "MGLFillExtrusionStyleLayer.h"
        ^

make darwin-style-code doesn’t automatically add MGLFillExtrusionStyleLayer.h to the project for you. Make sure to follow these steps (and the corresponding macOS steps) to make the header public.

@jfirebaugh jfirebaugh force-pushed the circle-stroke branch 3 times, most recently from 711cae3 to d382460 Compare December 8, 2016 22:48
Copy link
Copy Markdown
Member

@tobrun tobrun left a comment

Choose a reason for hiding this comment

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

Ran the smoke tests locally, 🚢

@jfirebaugh
Copy link
Copy Markdown
Contributor Author

jfirebaugh commented Dec 9, 2016

@tobrun Can you see why the Android build is failing?

[Edit: never mind, I think I figured it out.]

Copy link
Copy Markdown
Contributor

@1ec5 1ec5 left a comment

Choose a reason for hiding this comment

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

There’s a bit more accounting to do in the iOS/macOS documentation comments, but for the most part this change looks good.

These changes don’t actually add support for 3D extrusions, do they? It’s going to be weird to have an MGLFillExtrusionStyleLayer type that’s effectively a no-op.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

After pulling in mapbox/mapbox-gl-style-spec#619, this will say circleRadius like it’s supposed to.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We override any documentation that says “sprite” to say “style image” in platform/darwin/scripts/style-spec-overrides-v8.json while we await mapbox/mapbox-gl-style-spec#220.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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 represented in the SDK.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Per #6107 (comment), we override documentation to remove the phrase “as rgba”, because this is CSS syntax, and alpha may be specified with -colorWithHue:saturation:brightness:alpha: or -colorWithWhite:alpha: instead of -colorWithRed:green:blue:alpha:.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It’s kind of unfortunate that we’d be introducing references to light properties before actually adding that API to the SDK.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Would be nice to say “fill extrusion” instead of “fill-extrusion” here, but that can happen in a separate PR.

@jfirebaugh jfirebaugh force-pushed the circle-stroke branch 2 times, most recently from bd8543e to 3bef308 Compare December 9, 2016 00:46
@jfirebaugh
Copy link
Copy Markdown
Contributor Author

Ok, I manually reverted all the fill-extrusion additions. It's a booby trap for whoever next runs make style-code, but I don't want to polish the docs right now. I'm just trying to ship DDS.

@1ec5
Copy link
Copy Markdown
Contributor

1ec5 commented Dec 9, 2016

It's a booby trap for whoever next runs make style-code

If it’s a problem, we could special-case fill-extrusion for now and delete it from the spec approximately here.

@jfirebaugh jfirebaugh merged commit 82856fd into master Dec 9, 2016
@jfirebaugh jfirebaugh deleted the circle-stroke branch December 9, 2016 17:35
1ec5 added a commit that referenced this pull request Jan 16, 2017
@boundsj boundsj added this to the ios-v3.5.0 milestone Jan 18, 2017
1ec5 added a commit that referenced this pull request Jan 19, 2017
Updated changelogs to mention #7446, #7356, #7465, #7616, #7445, #7444, #7526, #7586, #7574, and #7770. Also corrected the blurb about #7711.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants