Support new style format

[edenh]

Now that mapbox/mapbox-gl-js#363 is merged, tracking this task here.

[incanus]

👍

I'm going to leave it off the WWDC milestone for now unless not moving to it before then presents a problem or we have sufficient time to do it.

[edenh]

It would allow us to move quicker on #171, but we can punt if it's a distraction for wwdc.

[incanus]

Ah, ok. Adding to the milestone, then, so we can plan accordingly. Can always drop if it proves too tricky for the time left.

[incanus]

I'm going to try to get an ETA on this today, as @edenh says this will greatly increase debug turnaround time.

[edenh]

A stopgap fix for this could be a gl-style script that converts from v1 backwards to v0. Not sure if this is worth pursuing. cc @mourner

[mourner]

I could write a backwards script — shouldn't take more than a day, although it seems to me that supporting new style format in llmr-native isn't a lot of work either.

[ghost]

I'm starting to take a look at this.

Some notes:

• Commit with the v0 style docs is here, and @mourner plans to update them to v1 soon
• The current style spec and migration scripts are here
• The work to be done on this probably will all go in style_parser.cpp
• "...though it shouldn't impact what you do, the xcode build phase bakes style.js into a string in resources.cpp ... and that's what is parsed. minifies it, basically"
• "@mourner reworked the json format a few weeks back and it's what we use on webgl ... we are still using our original dev format for native so folks have been having to backport to test on native"
• "check out https://github.com/mapbox/gl-style ... there are examples of the two formats in the test folder"
• "it's not really tm2, it's hand-crafted in a certain sense ... it's the mapping of sources (of tiles) to buckets to style them as (roads as lines) along with cascading classes with properties"
• "the sources / tile urls themselves are specified in map.cpp - that shouldn't change for this work though"
• "a quick way to play is to grab the gl-ported-styles repo and open the self-contained index.html in there and edit the style.js yourself & reload in browser"
• A bucket is "a structure that vector tile features like points and lines are added to, in tile-space ... then, buckets render themselves in gl ... so there are line and fill and point etc buckets ... a tile will have one of each type of feature it includes, no, one for each defined in the style"
• Take a look at https://github.com/mapbox/llmr-native/blob/master/src/renderer/point_bucket.cpp

[edenh]

@wsnook just incase you needed even more context, there is mapbox/mapbox-gl-js#356

[ghost]

@edenh Oh, nice. Context is my friend. Thanks.

[kkaefer]

The general idea of the new style format is to reduce the conceptual complexity.

Basically, what used to be buckets is now part of the styles and classes. Note that we still need to generate the bucket objects, but we implicitly deduce them from styles and the associated class from classes.

For generating the correct buckets, we have to go through the structure, and pull the associated style (with structure.name == style.name) from the default class, then extract all the parse-time properties (like line-cap, line-join, text-path, text-max-size, text-field and so on (some of which we currently don't support, btw)) and write them to the bucket description along with the filters from the structure array, and create a style class from the rest. Note that we don't have type fields anymore, so we have to look at what sort of properties the class has.

@mourner When a class specifies fill-* and line-* properties, do we create two buckets and two styles? What's the JS semantics here?

[kkaefer]

Also, we need to do some bucket deduplication, i.e. filter out buckets that have the same set of filters + properties, and make the structure objects link to a single bucket. This is important so that we don't duplicate all of the geometry for things like offset building shadows.

[edenh]

I was curious so I ran the following v1 stylesheet through the converter:

{
    "version": "1",
    "layers": [
        {"id": "background"},
        {
            "id": "road_label", 
            "filter": {
                "source": "source", "layer": "road_label"
            }
        }
    ],
    "styles": {
        "default": {
            "background": {
                "fill-color": "red"
            },
            "road_label": {
                "point-color": "blue",
                "point-radius": 10,
                "text-field": "name",
                "text-color": "blue",
                "text-size": 10
            } 
        }
    }
}

output:

{
  "version": "1",
  "buckets": {
    "road_label": {
      "filter": {
        "source": "source",
        "layer": "road_label"
      },
      "point": true,
      "text": true,
      "text-field": "name"
    }
  },
  "layers": [
    {
      "id": "background"
    },
    {
      "id": "road_label",
      "bucket": "road_label"
    }
  ],
  "styles": {
    "default": {
      "background": {
        "fill-color": "red"
      },
      "road_label": {
        "point-color": "blue",
        "point-radius": 10,
        "text-color": "blue",
        "text-size": 10
      }
    }
  }
}

Instead of two buckets for road_label, there is a single bucket with two types defined.

Also, bucket deduplication is already happening. https://github.com/mapbox/gl-style/blob/master/test/styles/bright-raw.js#L58-L60 is a single building bucket, while 3 styles reference it: https://github.com/mapbox/gl-style/blob/master/test/styles/bright-raw.js#L384-L393

In the v1 style, three building layers are defined: https://github.com/mapbox/gl-style/blob/master/test/styles/bright-v1.js#L44-L53, which in my opinion is preferable so that there is a direct relationship via id's.

[mourner]

@kkaefer the idea is to have one bucket that can specify several types. Currently llmr doesn't handle multiple types at once, but could be updated to do this pretty easily I assume.

As @edenh mentioned, buckets are deduplicated, it just doesn't handle the case where buckets with the same filter but different properties (like line-cap) get merged into one — it just chooses one of the values for the merged bucket. We could instead only merge completely identical buckets, and create different buckets if the properties are different (and maybe throw a warning that it's not recommended because it slows things down). I don't think it's a critical issue though, as I don't yet know any practical cases where you would want to have different bucket properties, as double buckets are currently only used for casings. cc @ansis

[ghost]

I think I'm finally understanding how all this stuff fits together. It seems like the most straightforward way to keep native in sync with web would be a pretty direct port of the js changes from mapbox/llmr#363 over to native.

@incanus I think this probably needs to go a bit beyond what you were saying about the changes being confined to style_parser.cpp. Am I missing something obvious?

[incanus]

No, that's sounding right. I didn't understand the bucket changes earlier.

[ghost]

For the native port to catch up to web, these changes need to be made:

• New style format mapbox-gl-js#363
  • debug/style.js
  • js/geometry/bucket.js
    • Promote text, point, line, and fill from values to keys
    • Change the syntax for point-size from a dictionary to an array
    • Change spacing to point-spacing
    • Change padding to point-padding
    • Add bucketFilter comparison for source and feature_type
    • Change join to line-join
    • Change cap to line-cap
    • Change miterLimit to line-miter-limit
    • Change roundLimit to line-round-limit
  • js/render/drawcomposited.js
    • Change name to id
  • js/render/drawfill.js
    • Change layer color to fill-color
    • Change layer stroke to stroke-color
  • js/render/drawline.js
    • Change line color to line-color
    • Change line width to line-width
    • Change line offset to line-offset
    • Change image to line-image
    • Change how the shader draws lines
  • js/render/drawpoint.js
    • Change point image to point-image
    • Change point color to point-color
    • Change point radius to point-radius
    • Change point invert to point-invert
    • Change point alignment to point-alignment
    • Change point rotate to point-rotate
  • js/render/drawtext.js
    • Change text path to text-path
    • Change text rotate to text-rotate
    • Change text size to text-size
    • Change text fontSize to text-max-size
    • Change text color to text-color
    • Change text stroke to text-halo-color
  • js/render/painter.js
    • Change name to id
    • Change test from bucket === 'background' to id === 'background'
    • Change class structure for bucket to reflect that text, point, line, and fill are now keys not values
    • Determine the bucket type, taking into account that text, point, line, and fill are now keys instead of values of type
    • Fix the warning about no bucket type being specified
  • js/style/bucket-filter.js
    • Implement bucket filtering
  • js/style/style.js
    • Change style's structure to layers
    • Change how line color pre-multiplication works
    • Change bucket.source to bucket.filter.source
    • Change name to id
    • Update how classes and styles work
      • Change classes key to styles
      • Change classes syntax (value is array) to styles syntax (value is dictionary)
      • Get rid of name key and promote its old values, default, satellite, and test to keys or, do it in a more general way where all top level keys become the name of a style.
  • js/style/styledeclaration.js
    • Change the structure of the javascript parsing engine to reflect multiple renamed keys
    • Change linear function to take a dictionary of parameters instead of an array
    • Change exponential function to take a dictionary of parameters instead of an array
    • Change how the min() and stopsFn() functions work
  • js/style/styletransition.js
    • Change how the interpolation of opacity, colors, line parameters, point parameters and raster parameters works for style transitions
  • js/text/placement.js
    • Change path to text-path
    • Change padding to text-padding
    • Change maxAngleDelta to text-max-angle
    • Change textMinDistance to text-min-distance
    • Change rotate to text-rotate
    • Change fontSize to text-max-size
    • Change alwaysVisible to text-always-visible
  • js/ui/geojsontile.js
    • Change how filtering works for feature types
  • js/ui/map.js
    • Change color to fill-color
  • js/worker/worker.js
    • Change how bucket filtering works
  • js/worker/workertile.js
    • Change how features are sorted out of tile layers into buckets
  • test/styledeclaration.test.js
    • Change width to line-width

... and potentially also this:

• mapbox/mapbox-gl-js@7f76cc6
  • js/ui/rastertile.js

[ghost]

Note to self: do not trust the syntax of the mapbox/gl-style v1 test file because it is rather different than what llmr is actually using after the newstyle merge. In particular, note the layers vs buckets difference.

@mourner Does this sound right?

/cc @edenh

[mourner]

@wsnook v1 version is user-friendly, while the script can convert v1 styles to renderer-friendly styles that have buckets and stuff. These have -raw prefix in the test samples.

[ghost]

@mourner So your expectation is that designers will always run the script and that llmr & llmr-native should expect the raw format?

[mourner]

@wsnook the preprocessing script is also a part of tm2-gl, a visual map style editor now in development, so designers won't have to do this manually.

[ghost]

@mourner Okay, cool. That makes a lot of sense, and I'm pretty sure I understand now how the pieces need to fit together.

[ghost]

Status update: I'm making good progress at annotating the specific changes which were made to llmr's style format as a part of PR mapbox/mapbox-gl-js#363, and I'm using that to fill out my giant checklist (a few comments up) of all the adjustments which must be made to llmr-native's style format.

The checklist will be useful for ensuring all the changes get made, and I expect it could potentially also be helpful for writing documentation about what changed.

[ghost]

@mourner Awesome. I just grabbed a copy of that.

I've added a sample of v1 outdoors to test styles in gl-style repo yesterday, check it out.

[ghost]

I'm getting very suspicious that llmr-native's v0 style parser ignores many of the v0 keys which were supported by llmr before the new style format change. If you scroll up a bit and look at my checklist of changes that I've made, you'll see a lot of holes for things like spacing, padding, line offset, and line image. The keys I just mentioned don't appear to be present anywhere in the llmr-native parser when I do string searches.

/cc @edenh @incanus @kkaefer @mourner

[incanus]

The keys I just mentioned don't appear to be present anywhere in the llmr-native parser when I do string searches.

You're right -- llmr-native's parser wasn't up to v0 complete when the new v1 style was migrated to. We're still playing catch-up.

[ghost]

We're still playing catch-up.

Okay, cool. I was getting a little confused there for a while.

[ghost]

Status Upate: So far I'm checking lots of changes off my todo list, and the app is not crashing, but it's also showing an empty white map. My expectation is that at some point it will magically start looking like an 80% correct outdoors map, then the question becomes how hard is it to get the remaining stuff looking right? I'm optimistic this can be finished by the end of the week, but there could easily be some time consuming obstacles that I can't see from here.

[ghost]

Just a heads up... My issue172-newstyle branch will end up conflicting in src/style/style_parser.cpp with #140 (Implement nested layers) and maybe also #191 (Support comp-op blend modes). If #140 merges into master soon, I'll merge it into my branch and try to fix the conflicts there.

/cc @edenh @kkaefer

[ghost]

For tomorrow, the next thing I need to do is get source and layer and class working for bucket's filter. This is one obvious reason, and hopefully the last, why I'm still getting a white map.

[ghost]

Bucket filtering works, and it draws a map now! Granted, it's kind of a crazy looking map... the fact that nested layers are not supported yet is definitely a problem with the v1 outdoors test style, and there might easily be other problems. I'm hoping that maybe merging this with #140 will magically fix some or all of the map craziness.

[ghost]

@edenh @incanus @kkaefer As I'm digging through all the notifications and new code this morning to take the pulse of where things are at getting ready for WWDC, I get the strong sense that we're out of time for doing the new style format for WWDC. I think it's time to punt on this.