Map not displayed - Githubissues

IagoLast commented 7 years ago

Regarding to this map:

https://mamataakella.carto.com/builder/e623bb0c-b2fd-11e5-b55f-0ea31932ec1d/embed

After some research, I found that is something related to the marker-size.

The following includes the 2 scene.yaml files that can be tested in the tangram.play https://gist.github.com/IagoLast/a0c085c71db4e43829f733f60d84caad , notice that the original size property layers>hash>data>draw causes tangram to crash.

EDIT 1

I found 2 bugs in tangram-carto

Default value bug

There is a helper function in tangram-carto which creates aux functions for variable sizes/colors..etc

This helper function sets the default value to null, generating problems when the value is not overwritten. In the case of this map, for the color, we have the following:

function() {
  var _value = null; // Default value --> null
  if ((63 & (1 << $zoom)) && feature['outbreaks'] >= 500) {
    _value = feature['symbol_size'];
  }
  if ((8 & (1 << $zoom)) && feature['outbreaks'] >= 500) {
    _value = feature['symbol_size'] * 2;
  }
  if ((16 & (1 << $zoom)) && feature['outbreaks'] >= 500) {
    _value = feature['symbol_size'] * 4;
  }
  return _value; // Sometimes this function returns null causing a tangram error!
}

Colide bug

Acording to the tangram doc must be a boolean field.

Tangran-carto gets the collide value from the getCollide(c3ss).

In the case of this map, this function returns 0 instead "true" so the map is not looking well.

As long as I know the getCollide (which value for this map must be false, gets the value from the maker-allow-overlap field in the cartocss which now has the following value:

{
  "constant": true,
  "symbolizer": "markers",
  "js": [
    "if((63 & (1 << ctx.zoom)) && data['outbreaks'] >= 500){_value = true;}"
  ],
  "index": 285
}

cc @alonsogarciapablo @rochoa

IagoLast commented 7 years ago

The bug 1 can be fixed changing the default value from null to 0. (and praying).

I need more context to solve the bug 2.

alonsogarciapablo commented 7 years ago

Great job investigating this @IagoLast !

IagoLast commented 7 years ago

Forget about the collide bug. Its a consequence of changing the default value.

If the default value remains to "null" there´s no problem so mi previous proposal of changing the default value from null to 0 is discarded.

IagoLast commented 7 years ago

Final report.

The are 2 bugs here, one related to the marker-with default value and another one related to marker-allow-overlap property.

Bug1: Marker with default value

In the attached map, the cartocss is the following:

#layer_name [outbreaks >= 500][zoom <= 5]{
  marker-fill-opacity: 0.5;
  marker-line-color: #FFF;
  marker-line-width: 0.25;
  marker-line-opacity: 1;
  marker-fill: #fff;
  marker-allow-overlap: true;
  marker-width: [symbol_size]; // <---

  [zoom = 3]{
    marker-width:[symbol_size] * 2;
  }
...

As you can see, marker-with is only set when outbreaks >= 500 and zoom <= 5 causing the error explained in the edit1

IMO an issue should be created in order to give the right default value in the size function.

Bug 2: marker-allow-overlap

According to the tangram-doc collide must be a BOOLEAN field, this means that no functions can be used here.

Again, if we check the css we can see that the marker-allow-overlap rule is depends on the zoom and the outbreaks values. This mean that at tangram-css level will be translated to a function that tangram will ignore.

IMO: The fix for this would be a fallback to raster when allow-overlap is included under a conditional rule. and ask tangram to support functions in the collide field.

Note: This two bugs dissapear with little modifications in the cartocss:

#layer_name {
  marker-width: 0; // We give a default value for this
  marker-allow-overlap: true;  // Allow overlap is not conditional anymore!

  [outbreaks >= 500][zoom <= 5]{
    marker-fill-opacity: 0.5;
    marker-line-width: 0.25;

  }

  [ zoom = 3] {
      marker-width:[symbol_size] * 2;
  }

The vector result can be checked here

cc @rochoa @alonsogarciapablo

makella commented 7 years ago

wow, thanks so much for looking into this and the detailed breakdown! good things to know :)

alonsogarciapablo commented 7 years ago

Great job investigating and summarising what's going on here @IagoLast!

I totally agree with what you're suggesting here @IagoLast. I'd close this issue, split it into two different issues and:

Fix Bug #1 and make sure this library sets a valid default value for each of the properties (eg: the one defined in github.com/cartodb/tangram-reference). It looks like this is not something specific to marker-width and this could potentially fail with other properties as well...
Open an issue in https://github.com/tangrams/tangram and explain them that we need support for functions in colide.... this could just work if Tangram changes this so Bug #2 could probably wait until we hear from them.

@makella is it very common to change marker-allow-overlap conditionally? Example:

#layer_name {
  ...
  marker-allow-overlap: true;

  [zoom = 3] {
    ...
    marker-allow-overlap: false;
  }

IagoLast commented 7 years ago

Regarding to the issue 1

I created a hotfix for the first problem in https://github.com/CartoDB/tangram-carto/pull/56 , a good solution implies lots of changes (about 2 weeks with my current project knowledge)

IagoLast commented 7 years ago

Regarding to the issue 2

I created an issue in the tangram repo and they said they wont implement collide function because of performance reasons.

They suggest a different approach to this kind of things using filters instead functions.

I think we should study this approach but in any case solving this will take time.

IagoLast commented 7 years ago

Finally after discussing the issues with @javisantana we agreed to document the following:

Conditional symbolizer activation is not supported: This means that if you want the markers to be invisible you have to set a 0 size or a transparent color.
Dynamic marker-allow-overlap is not supported: this means that you cannot change the marker-allow-overlap value depending on feature/zoom properties.

javisantana commented 7 years ago

I'd try to find those cases before rendering and return the error on parse time (so we can fallback to raster)

IagoLast commented 7 years ago

@javisantana to detect this cases at parse time, the CartoCSS compiler needs to be aware of what cases are not supported. Currently the CartoCSS compiler knows some limitations using the json reference, but this reference can only express simple rules such as non existent property or invalid type for a given property...

The only way to achieve this would be changing the compiler code to detect those cases. As long as I know the compiler is being used in different projects with different subsets of supported features so it don't seems like a good idea to me to change the compiler.

IMO the responsibility of the compiler should be parse and validate the cartocss, since this cases are valid cartocss (but they are not supported by tangram) it must be tangram-carto the one who throws an error pointing that the cartocss cannot be rendered.

(Note: The golden price solution here would be to have a general compiler that allows a grammar as an input. With a grammar we can change everything we need and we at compile time. Check: https://github.com/harc/ohm or https://github.com/pegjs/pegjs)

javisantana commented 7 years ago

the problem is the make the decision raster vs vector the only two things we are taking into account is: 1) the parser parses the cartocss (with tangram reference) 2) the number of features is low enough for client side

But obviously that's not enough, we need, at least, a third one.

3) the renderer support the style

So you are right, the compiler/parser does what it does, but I'm not saying the parser should do that

IagoLast commented 7 years ago

the renderer support the style

any ideas about how to address this?

javisantana commented 7 years ago

my proposal:

to check if there is more than one marker-allow-overlap we should: 1) modify the carto parser to include some metadata. Right now it gives you some metadata, we could include if the attribute depends on attributes.

var cartocss = [
    '#layer {',
    ' marker-allow-overlap: true;',
    ' [cat = 1] { marker-allow-overlap: false; }',
    '}'
].join('\n')
new carto.RendererJS().render(cartocss).getLayers()[0].shader['marker-allow-overlap'].DEPENDSON_ATTRIBUTES

For the second case is almost the same, you should check if all the propierties for the same symbolizer have a "default path" (I know that can be known at parsing time)

function checkDefaultPath(cartocss) {
  var shader = new carto.RendererJS().render(cartocss).getLayers()[0].shader;

  for(var k in shader) {
    if (shader[k].symbolyzer === 'marker' && !shader[k].HAS_DEFAULT_PATH) {
       return false;
    }
  }
  return true
}

IagoLast commented 7 years ago

Fixed in:

https://github.com/CartoDB/tangram-carto/issues/58

and

https://github.com/CartoDB/tangram-carto/issues/61

CartoDB / tangram-cartocss

Map not displayed #55

EDIT 1

Default value bug

Colide bug

Final report.

Bug1: Marker with default value

Bug 2: marker-allow-overlap

Note: This two bugs dissapear with little modifications in the cartocss:

Regarding to the issue 1

Regarding to the issue 2