hotosm / HDM-CartoCSS

CartoCSS project focused on the Humanitarian Data Model
Other
138 stars 41 forks source link

oneway=no renders arrows #198

Closed ghost closed 10 years ago

ghost commented 10 years ago

The selector for oneway streets is currently in labels.mss as follows:

532  #motorway_label[oneway!=null][zoom>=16],
533  #mainroad_label[oneway!=null][zoom>=16],
534  #minorroad_label[oneway!=null][zoom>=16] {

oneway=no obviously also gets selected by this.

The same probably goes for shops (poi.mss|85 and labels.mss|365) and crafts (poi.mss|134 and labels.mss|330).

skorasaurus commented 10 years ago

oneway=no obviously also gets selected by this

If you just look at this piece of carto, then yes, you would assume oneway=no is selected. However, the sql query for the minorroad_label layer only selects those that have a oneway value of -1, 1, true, and yes.

Reexamining this, there is a related bug here: the minorroad_label will select ways if it name is not null OR a oneway's column has a value of -1, 1, true, or yes.

So, if a highway has any name and is tagged oneway=no (which is not a correct tag but it is used often in osm) the oneway arrows will appear on it. An example of this is Rue 2 http://www.openstreetmap.org/browse/way/112377267

A quick fix could be done by changing in labels.mss to [oneway!='no'] however, if I remember correctly, using regex negatives can hamper performance and speed, so I want to check with one of the carto devs or w/ @yohanboniface and test it before committing it.

@yohanboniface or anyone else, feel free to fix this, otherwise, I can, in about 8-10 hours from now.

Regarding, shops and crafts: there's many shops and crafts that don't have dedicated icons yet but are important to include. Since there isn't an established way of tagging many of these yet (besides a craft or shop tag), we decided to include all of them. In those cases, I guess we could change it to 'shop!='no' although we may run into same problem with the regex.

skorasaurus commented 10 years ago

So, I can confirm that changing it to 'oneway!='no' will fix the bug but do we want to fix it this way ?

Or we could replace that with selectors for top three values of oneway

[oneway='yes'][oneway='true'][oneway='1']

We're already using a regex here so performance/rendering speed won't decrease but after talking to @springmeyer confirmed my recollection that regex can hurt performance in general.

danstowell commented 10 years ago

Hi - Andy Allen has always told me that it's important to render only the tags you want, and not to sweep up weird extra tags in by using "anything else" selectors. From that perspective, "oneway!=no" is bad - it renders things like "oneway=definitely" or "oneway=fixme", so it fails to encourage people to use the proper tagging. Therefore, I advocate the top-three-values method.

(Note, also -1 should be handled, with arrows the opposite way. You've not mentioned it in the "top three" even though it's the third most popular - is that because we are preprocessing it by sql or suchlike?)

ghost commented 10 years ago

I'm not familiar with all the SQL code and the like, but from how I understand this, -1 should be in (what then becomes) the top four.

#motorway_label[oneway!=null][zoom>=16],
#mainroad_label[oneway!=null][zoom>=16],
#minorroad_label[oneway!=null][zoom>=16] {
  marker-placement:line;
  marker-max-error: 0.5;
  marker-spacing: 200;
  marker-file: url('icons/oneway.svg');
  [oneway=-1] { marker-file: url('icons/oneway-reverse.svg'); }
  [zoom=16] { marker-transform: "scale(0.5)"; }
  [zoom=17] { marker-transform: "scale(0.75)"; }
}
yohanboniface commented 10 years ago

We should always select and prepare data in the layers, and style the data with carto. So carto is only for targeting, not for selecting.

Also, I think we don't care about "1" and "true", according to taginfo stats and wiki (imho, renderers also (have to) do their part of the job of discouraging editors to use wrong values, even if we don't tag for [censored]).

Anyway, I'm going to merge the #199 PR, and add a little SQL case to narrow the values known by carto.