Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Interaction of layer 'visible' flag with sublayers is unspecified #198

Open
matteblair opened this issue Mar 15, 2017 · 10 comments
Open

Interaction of layer 'visible' flag with sublayers is unspecified #198

matteblair opened this issue Mar 15, 2017 · 10 comments

Comments

@matteblair
Copy link
Member

https://github.com/tangrams/tangram-docs/blob/gh-pages/pages/layers.md

Example:

layers:
  parent-layer:
    visible: false
    child-layer:
      visible: true
      draw:
        polygons:
          order: 1
          color: blue

Is child-layer drawn? The current documentation is ambiguous on this, and we've recently discovered that tangram-js and tangram-es resolve this differently: tangrams/walkabout-style#154

cc @bcamper @nvkelso @meetar

@meetar
Copy link
Member

meetar commented Mar 15, 2017 via email

@matteblair
Copy link
Member Author

I wouldn't be surprised if this conversation has happened before :)

As I see it, the layer visible flag would be more useful as a "master switch" precisely because it behaves differently from draw rules (which are merged bottom-up). As you said, without it there is no way to confidently disable an entire branch of the layer tree. Also, it isn't really the case that "everything else" uses the bottom-up-merge behavior, it's only draw rules that do that. Filters, for example, are a layer property that act more-or-less like a "master switch" the way you're describing.

@nvkelso
Copy link
Member

nvkelso commented Mar 15, 2017

There are cases when both would be useful in the basemap styles ;) Something to just turn a whole layer and it's children off (enabled: true?) versus the useful visibility cascading – which is necessary for overlays and label variants.

@matteblair
Copy link
Member Author

The "cascading" visibility is already available in a way with the visible draw parameter (which is distinct from the visible layer flag). It isn't exactly the same, but it's close enough that it seems almost redundant to have the layer visible flag behave the same way.

@matteblair
Copy link
Member Author

(I am admittedly prejudiced towards the "master switch" behavior because that's how it is already implemented in tangram-es).

@meetar
Copy link
Member

meetar commented Mar 15, 2017 via email

@tallytalwar
Copy link
Member

tallytalwar commented Mar 15, 2017

+1 on enabled for layer property, defaulting to true.

@matteblair
Copy link
Member Author

I think there's evidence now that the two parameters are easily confused, a different name would probably go a long way to prevent that.

I suspect that the layer visible flag is not frequently used in scenes, since it has had different behaviors in JS and ES and we didn't notice until now.

In the 250 usages of visible: in bubble-wrap, I found exactly one instance where it was used as a layer flag (and was redundant).

@bcamper
Copy link
Member

bcamper commented Mar 16, 2017

I've come around to the idea of a "master layer" switch both because it provides a function we don't have now (a simple way to cull whole sections of the layer tree), and also because it can provide possibly significant run-time savings by allowing the engine to stop processing layers at that point (previously JS has had to keep descending into all sub-layers in case one of them sets visible back to true).

I get the argument for changing the property name to enabled -- would we make it backwards compatible with visible for at least one release cycle though? (Even if the behavior changes a bit.)

I would not like to have both behaviors (overridable and non-overridable) at the layer level (too much complexity); as pointed out, we still have the ability to override the visible parameter within draw groups for achieving this.

That said, I notice that the demo bundled with Tangram JS makes a lot of use of layer visible, and the pattern of turning it on only at the sub-layer level. Are these rendering differently in ES, but we haven't noticed yet? For example: https://github.com/tangrams/tangram/blob/master/demos/scene.yaml#L627-L651

@matteblair
Copy link
Member Author

I agree that if we choose a new name for this parameter then we should maintain backward compatibility with visible for some amount of time.

It seems that the current demo scene bundled with Tangram JS does indeed render differently in Tangram ES (we use default scene that is similar but not identical, so we wouldn't have noticed before now).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants