search

tangrams / bubble-wrap

Bubble Wrap basemap style
http://tangrams.github.io/bubble-wrap/
MIT License
27 stars 21 forks source link

Ambiguous draw parameters #112

Open matteblair opened 8 years ago

matteblair commented 8 years ago

The layers pois_and_landuse_labels:has-name:default-label-text and pois_and_landuse_labels:has-name:bank-early* can both match the same feature and assign the visible parameter to the text rule with different values at the same depth.

There is no way for tangram to determine which rule should "win" in cases like this and therefore it is totally valid for an implementation to choose either value at random. This is a known limitation of the stylesheet syntax. To get well-defined behavior, some of these rules will have to be removed or re-structured such that they don't collide.

*= The nearby layers 'office-early', 'post-office-early', 'atm-early', 'memorial-early', and 'parking-labels-early' will almost certainly have this problem as well.

matteblair commented 8 years ago

After adding an automated check for rule ambiguity and exploring the map, I've found that this problem is more pervasive than I'd thought. Here is a non-exhaustive list of ambiguous rules that I've encountered:

To me this indicates that it is far easier to write ambiguous rules than we had anticipated. Short of changing the stylesheet syntax, one thing that we can do to improve the situation is to enforce that tangram resolves these conflicts in a consistent way. Even if conflicts are resolved in a completely arbitrary way, this would prevent the problem we have now where styles authored in one renderer can be validly interpreted differently in another renderer.

@bcamper I now think that addressing this should be the highest priority task for both tangram implementations right now. Let's work together on a way to resolve these conflicts consistently!

bcamper commented 8 years ago

Yes, I think we need to work within the current syntax for now, as it would be a much bigger refactor (and a syntax change I don't want to rush) to change that behavior.

I will look into the easiest way to accomplish this in JS. Lexical sort by layer name may be the most straightforward (which doesn't need the fully qualified layer name, because this only needs to be resolved for rules at the same depth, right?).

On Sat, Dec 5, 2015 at 4:15 PM, Matt Blair notifications@github.com wrote:

After adding an automated check for rule ambiguity and exploring the map, I've found that this problem is more pervasive than I'd thought. Here is a non-exhaustive list of ambiguous rules that I've encountered:

  • order in rule lines in layer roads:z-order conflicts with layer roads:highway
  • order in rule lines in layer roads:z-order conflicts with layer roads:path
  • order in rule lines in layer roads:z-order conflicts with layer roads:track
  • sprite in rule icons in layer pois_and_landuse_labels:has-name:station-train-subway conflicts with layer pois_and_landuse_labels:has-name:direct-match
  • visible in rule icons in layer pois_and_landuse_labels:has-name:direct-match conflicts with layer pois_and_landuse_labels:has-name:bank-early
  • visible in rule text in layer pois_and_landuse_labels:has-name:default-label-text conflicts with layer pois_and_landuse_labels:has-name:bank-early
  • outline:color in rule lines in layer roads:path:in_park conflicts with layer roads:path:pedestrian
  • color in rule polygons in layer buildings:in_university conflicts with layer buildings:extrude
  • color in rule polygons in layer buildings:in_hospital conflicts with layer buildings:extrude
  • color in rule polygons in layer buildings:in_park conflicts with layer buildings:extrude
  • visible in rule icons in layer pois_and_landuse_labels:has-name:direct-match conflicts with layer pois_and_landuse_labels:has-name:atm-early
  • visible in rule text in layer pois_and_landuse_labels:has-name:default-label-text conflicts with layer pois_and_landuse_labels:has-name:atm-early
  • sprite in rule icons in layer pois_and_landuse_labels:has-name:icons:mosque conflicts with layer pois_and_landuse_labels:has-name:icons:spiritual-center
  • font:fill in rule text in layer roads:service_road:labels-service_road2 conflicts with layer roads:service_road:labels-service_road
  • font:size in rule text in layer roads:service_road:labels-service_road2 conflicts with layer roads:service_road:labels-service_road
  • sprite in rule icons in layer pois_and_landuse_labels:has-name:icons:buddhist conflicts with layer pois_and_landuse_labels:has-name:icons:spiritual-center
  • sprite in rule icons in layer pois_and_landuse_labels:has-name:icons:synagogue conflicts with layer pois_and_landuse_labels:has-name:icons:spiritual-center
  • sprite in rule icons in layer pois_and_landuse_labels:has-name:icons:church conflicts with layer pois_and_landuse_labels:has-name:icons:spiritual-center
  • outline:color in rule lines in layer roads:path:in_cemetery_garden conflicts with layer roads:path:in_park
  • visible in rule icons in layer pois_and_landuse_labels:has-name:direct-match conflicts with layer pois_and_landuse_labels:has-name:parking-labels-early
  • visible in rule text in layer pois_and_landuse_labels:has-name:default-label-text conflicts with layer pois_and_landuse_labels:has-name:parking-labels-early
  • color in rule lines in layer roads:major_road:secondary conflicts with layer roads:major_road:tunnel
  • outline:color in rule lines in layer roads:major_road:secondary conflicts with layer roads:major_road:tunnel
  • color in rule lines in layer roads:major_road:tunnel conflicts with layer roads:major_road:trunk_primary
  • outline:color in rule lines in layer roads:major_road:tunnel conflicts with layer roads:major_road:trunk_primary
  • visible in rule icons in layer pois_and_landuse_labels:has-name:direct-match conflicts with layer pois_and_landuse_labels:has-name:university-poi
  • visible in rule text in layer pois_and_landuse_labels:has-name:default-label-text conflicts with layer pois_and_landuse_labels:has-name:university-poi
  • order in rule lines in layer roads:z-order conflicts with layer roads:aerialway
  • cap in rule lines in layer roads:minor_road:minor_road_bridge conflicts with layer roads:minor_road:round
  • order in rule lines in layer roads:highway:tunnel conflicts with layer roads:highway:not_link
  • outline:color in rule lines in layer roads:path:in_university conflicts with layer roads:path:pedestrian
  • outline:color in rule lines in layer roads:major_road:tertiary conflicts with layer roads:major_road:link
  • outline:color in rule lines in layer roads:path:in_university conflicts with layer roads:path:bridge
  • order in rule lines in layer roads:minor_road:tunnel conflicts with layer roads:minor_road:early
  • color in rule lines in layer roads:major_road:tertiary:in_park conflicts with layer roads:major_road:tertiary:routes
  • outline:color in rule lines in layer roads:major_road:tertiary:in_park conflicts with layer roads:major_road:tertiary:routes
  • outline:color in rule lines in layer roads:major_road:secondary:in_park conflicts with layer roads:major_road:secondary:routes
  • color in rule lines in layer roads:major_road:tertiary conflicts with layer roads:major_road:tunnel
  • outline:color in rule lines in layer roads:major_road:tertiary conflicts with layer roads:major_road:tunnel
  • color in rule dots2 in layer landuse:tier2:nature_reserve conflicts with layer landuse:tier2:conservation
  • outline:color in rule lines in layer roads:path:bridge conflicts with layer roads:path:pedestrian
  • order in rule dots2 in layer landuse:tier2:parks-and-national-forests-not-national-park conflicts with layer landuse:tier1:national_park
  • order in rule lines in layer roads:z-order conflicts with layer roads:airport-lines
  • outline:color in rule lines in layer roads:service_road:in_cemetery_garden conflicts with layer roads:service_road:in_park
  • color in rule polygons in layer buildings:in_retail conflicts with layer buildings:extrude
  • priority in rule text in layer places:populated-places conflicts with layer places:region
  • order in rule lines in layer roads:z-order conflicts with layer roads:natural_earth_highways
  • priority in rule text in layer places:populated-places conflicts with layer places:region-z6
  • priority in rule text in layer places:populated-places conflicts with layer places:region-z5
  • priority in rule text in layer places:populated-places conflicts with layer places:country-z4
  • priority in rule text in layer places:populated-places conflicts with layer places:region-z4
  • priority in rule text in layer places:populated-places conflicts with layer places:country-z3
  • priority in rule text in layer places:populated-places conflicts with layer places:country-z2

To me this indicates that it is far easier to write ambiguous rules than we had anticipated. Short of changing the stylesheet syntax, one thing that we can do to improve the situation is to enforce that tangram resolves these conflicts in a consistent way. Even if conflicts are resolved in a completely arbitrary way, this would prevent the problem we have now where styles authored in one renderer can be validly interpreted differently in another renderer.

@bcamper https://github.com/bcamper I now think that addressing this should be the highest priority task for both tangram implementations right now. Let's work together on a way to resolve these conflicts consistently!

— Reply to this email directly or view it on GitHub https://github.com/tangrams/eraser-map/issues/112#issuecomment-162248557 .

matteblair commented 8 years ago

Cool, thanks! If we were going to use layer name, I think it would have to be the fully-qualified layer name; two "sibling" layers could both have sublayers with the same name and conflicting rules.

nvkelso commented 8 years ago

Could it be the relative order within the scene file (versus layer name, which is also random)?

Sent from my handsful device.

On Dec 5, 2015, at 13:44, Matt Blair notifications@github.com wrote:

Cool, thanks! If we were going to use layer name, I think it would have to be the fully-qualified layer name; two "sibling" layers could both have sublayers with the same name and conflicting rules.

― Reply to this email directly or view it on GitHub.

tallytalwar commented 8 years ago

I am not sure relative order will work because yaml parsing of these layers is not deterministic, at least on the es side. On Dec 5, 2015 7:42 PM, "Nathaniel V. KELSO" notifications@github.com wrote:

Could it be the relative order within the scene file (versus layer name, which is also random)?

Sent from my handsful device.

On Dec 5, 2015, at 13:44, Matt Blair notifications@github.com wrote:

Cool, thanks! If we were going to use layer name, I think it would have to be the fully-qualified layer name; two "sibling" layers could both have sublayers with the same name and conflicting rules.

― Reply to this email directly or view it on GitHub.

— Reply to this email directly or view it on GitHub https://github.com/tangrams/eraser-map/issues/112#issuecomment-162260286 .

tallytalwar commented 8 years ago

And I agree with Matt, lexical order of fully qualified layer names will be required. On Dec 5, 2015 7:44 PM, "Varun Talwar" varun@mapzen.com wrote:

I am not sure relative order will work because yaml parsing of these layers is not deterministic, at least on the es side. On Dec 5, 2015 7:42 PM, "Nathaniel V. KELSO" notifications@github.com wrote:

Could it be the relative order within the scene file (versus layer name, which is also random)?

Sent from my handsful device.

On Dec 5, 2015, at 13:44, Matt Blair notifications@github.com wrote:

Cool, thanks! If we were going to use layer name, I think it would have to be the fully-qualified layer name; two "sibling" layers could both have sublayers with the same name and conflicting rules.

― Reply to this email directly or view it on GitHub.

— Reply to this email directly or view it on GitHub https://github.com/tangrams/eraser-map/issues/112#issuecomment-162260286 .

bcamper commented 8 years ago

Right, unfortunately it is not possible to determine order from a YAML mapping (or JS object/hash either for that matter). The use of lexical manner is not really because it's "logical", it's so that it's consistent and predictable. This is still a rule behavior that we want to avoid (but thinking about syntax alternatives is a bigger topic that can't be done on a short timeline).

I will have a working version of lexical rule sort in JS shortly.

On Saturday, December 5, 2015, Varun notifications@github.com wrote:

I am not sure relative order will work because yaml parsing of these layers is not deterministic, at least on the es side. On Dec 5, 2015 7:42 PM, "Nathaniel V. KELSO" <notifications@github.com javascript:_e(%7B%7D,'cvml','notifications@github.com');> wrote:

Could it be the relative order within the scene file (versus layer name, which is also random)?

Sent from my handsful device.

On Dec 5, 2015, at 13:44, Matt Blair <notifications@github.com javascript:_e(%7B%7D,'cvml','notifications@github.com');> wrote:

Cool, thanks! If we were going to use layer name, I think it would have to be the fully-qualified layer name; two "sibling" layers could both have sublayers with the same name and conflicting rules.

― Reply to this email directly or view it on GitHub.

— Reply to this email directly or view it on GitHub < https://github.com/tangrams/eraser-map/issues/112#issuecomment-162260286> .

— Reply to this email directly or view it on GitHub https://github.com/tangrams/eraser-map/issues/112#issuecomment-162260376 .

matteblair commented 8 years ago

In tangram-es it would be pretty easy to resolve conflicts by order within the file (yaml-cpp provides positional information about each node). This seems like it would be a more intuitive solution than sorting by layer name, but it sounds like this might not be technically possible in tangram-js?

bcamper commented 8 years ago

Ah, didn't know that was possible in the C++ yaml parser!

Unfortunately it is not currently possible with the JS yaml parser, because it uses plain JS objects, and there is no guaranteed order for those keys. So I think lexical order is the only practical solution for now (though I agree it isn't the most intuitive and I initially wasn't so into the idea.. just don't see an alternative).

I have an idea to try forking js-yaml and making it use ES6's native Map, which does provide a way to iterate keys in insertion order. But, I don't know the scope of that project, it could be quite large and would necessitate a major API change for YAML access, (and forking cost...) so it will have to wait for the future.

On Saturday, December 5, 2015, Matt Blair notifications@github.com wrote:

In tangram-es it would be pretty easy to resolve conflicts by order within the file (yaml-cpp provides positional information about each node). This seems like it would be a more intuitive solution than sorting by layer name, but it sounds like this might not be technically possible in tangram-js?

— Reply to this email directly or view it on GitHub https://github.com/tangrams/eraser-map/issues/112#issuecomment-162262196 .

bcamper commented 8 years ago

So with lexical sort, is the preferred behavior that first ('rule-a') or last ('rule-z') wins? :)

matteblair commented 8 years ago

No preference here. I'd try both on the eraser-map style and see if one produces noticeably fewer differences.

bcamper commented 8 years ago

There are probably better ways to solve this, but also occurred to me that we could have an optional priority set on the layer to allow precedence control where needed.

bcamper commented 8 years ago

BTW I looked at how CSS resolves conflicts like this, and later selectors overwrite earlier ones. As discussed that would be the preferred behavior here if it was easy to implement (unfortunately I still don't think it is, though I started taking a look at js-yaml for future functionality).

Of course, for CSS like Tangram, it's preferable not to have conflicting rules in the the first place, but we may need new or revised syntax since it looks like it's easy to write them accidentally. Though having live Tangram Play warnings for this could help a lot :)

matteblair commented 8 years ago

Just having some well-defined method of resolving conflicts will make a world of difference here, refinements on that method (priority, document order, etc) will be nice enhancements for later.

This raises a question about the documentation/specification: Does this layer-name-resolution behavior become part of the specified behavior for tangram? Or do we leave it officially unspecified to allow for possibly changing the resolution behavior in the future? (I lean towards the latter)

nvkelso commented 8 years ago

@blair1618 Can you please run this analysis again now that the stylesheet has been updated, and Tangram has been updated?