CartoDB / tangram-cartocss

⛔️ DEPRECATED Transform cartocss into a draw tangram object
https://cartodb.github.io/tangram-cartocss/
BSD 3-Clause "New" or "Revised" License
4 stars 3 forks source link

[DNM] Default values should follow what the reference specifies #64

Closed rochoa closed 7 years ago

rochoa commented 7 years ago

This is just a POC of what I would expect. This misses the reading of the default value from the reference in use.

Does it make any sense?

Reference: https://github.com/CartoDB/tangram-carto/issues/63

rochoa commented 7 years ago

With

#layer {
  marker-fill: red;
  marker-allow-overlap: true;
  [price > 100] {
    marker-width: 100;
  }
}

Raster/Mapnik

screen shot 2017-08-28 at 15 17 51

Vector/Tangram current behaviour

screen shot 2017-08-28 at 15 18 33

Vector/Tangram with this PR

screen shot 2017-08-28 at 15 19 24

As you see, we are still missing some other default values for other properties (marker-line-*).

With my changes we will have different behaviour for a CartoCSS like:

#layer {
  [price > 100] {
    marker-allow-overlap: true;
    marker-width: 100;
  }
}

But that's an advanced use, and the user could always use an alternative to hide the unfiltered properties:

#layer {
  marker-width: 0;
  [price > 100] {
    marker-allow-overlap: true;
    marker-width: 100;
  }
}
davidmanzanares commented 7 years ago

Yes, I think that although both (previous and new) are buggy behaviors, the new behavior is more user-friendly and it's less likely to occur.

This problem is probably occurring with the other symbolizers too, I think we should swap to this new behavior on them too.

alonsogarciapablo commented 7 years ago

@rochoa I'm sure you know it (and that's why you skipped one failing test) but this PR reopens https://github.com/CartoDB/tangram-carto/issues/61 (there's a good explanation here) as is. The hard thing to accomplish is to fix #61 and #63 at the same time.

rochoa commented 7 years ago

But https://github.com/CartoDB/tangram-carto/issues/61 is a wrong assumption. The default size should be what the reference says. See my previous comment.

davidmanzanares commented 7 years ago

The problem is that we are introducing a new bug, similar, but not the same, to #61 . Instead of setting it to 0, we will be setting it to the default value even when the symbolizer is not active.

alonsogarciapablo commented 7 years ago

I guess the assumption we're making in #61 is the same assumption that mapnik is doing when rendering raster tiles (i.e: feature is not rendered if symboliser is not active for a given context).

rochoa commented 7 years ago

I guess the assumption we're making in #61 is the same assumption that mapnik is doing when rendering raster tiles (i.e: feature is not rendered if symboliser is not active for a given context).

But "feature is not rendered" is not the same as "render the feature with width=0".

IagoLast commented 7 years ago

Assuming that a conditional marker-width is an advanced use I agree with the new approach.

we should also get the default values for the remaining properties.

rochoa commented 7 years ago

Assuming that a conditional marker-width is an advanced use I agree with the new approach.

I assume:

#layer {
  [price > 100] {
    marker-width: 100;
  }
}

It's an advanced use.

Where:

#layer {
  marker-fill: red;
  [price > 100] {    
    marker-width: 100;
  }
}

It's a more common one.

So I prefer to fail in the first one, and work properly in the second one.

alonsogarciapablo commented 7 years ago

I agree too. @rochoa @dv343 will you take care of this then?

rochoa commented 7 years ago

@alonsogarciapablo, yes.

rochoa commented 7 years ago

Closing this as it is addressed in #69.