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

[Feat] Masking based on a polygon #6552

Closed
felixpalmer opened this issue Jan 10, 2022 · 24 comments
Closed

[Feat] Masking based on a polygon #6552

felixpalmer opened this issue Jan 10, 2022 · 24 comments
Assignees
Labels
Milestone

Comments

@felixpalmer
Copy link
Collaborator

Target Use Case

Currently deck.gl has a ClipExtension which allows rendering of layers clipped to rectangular region. This feature proposes adds a new extension, MaskExtension, which performs masking based on an arbitrary polygon region.

The MaskExtension would enable richer visualizations as clipping only by a rectangle is of limited use.

Proposal

The MaskExtension can be added to a deck.gl layer in the standard fashion:

const maskExtension = new MaskExtension();
new SolidPolygonLayer({
  data,
  getFillColor: [255, 0, 0],
  extensions: [maskExtension],
  maskPolygon: [[0, 0], [0, 1], [1, 1], [1, 0], [0, 0]]
});

The MaskExtension adds the following props:

  • maskPolygon: polygon outline (format the same as that used by the SolidPolygonLayer data prop)
  • maskEnabled: set to false to disable masking
  • maskByInstance: whether to clip by pixel or by the geometry instance (analogous to ClipExtension.clipByInstance)

Below is a short video showing the MaskExtension in action.

Mask_demo

The demo is composed of a number of different layers using the MaskExtension in different fashions:

  • A GeoJsonLayer with US states. When a state is selected, its geometry is used for the maskPolygon
  • A SolidPolygonLayer covering the entire USA with a solid green fill. This is masked using the maskPolygon. Thus when Texas is selected, it appears to have a green background
  • A ArcLayer showing the links between sources and target (flights between cities). As the maskByInstance prop is true, all arcs originating inside the mask are showing in their entirety
  • A ScatterplotLayer showing a buffer around each target. As the maskByInstance prop is set to false, these regions (in white) are clipped to the mask by pixel, and thus appear to be within the selected state.
  • 2 ScatterplotLayers showing the sources and targets for the sources and targets. As the maskByInstance prop is true, the red and green circles are not clipped by pixel
@felixpalmer felixpalmer self-assigned this Jan 10, 2022
@felixpalmer felixpalmer added this to the 8.7 milestone Jan 10, 2022
@felixpalmer
Copy link
Collaborator Author

felixpalmer commented Jan 11, 2022

TODO

  • Implementation
  • QA & test
  • Render tests
  • Unit tests
  • Documentation
  • Example app
  • Website update
  • Codepen (for website)

@Pessimistress
Copy link
Collaborator

What happens if you need to apply the same mask to multiple layers? Is it more efficient to define the mask with a polygon layer?

@ibgreen
Copy link
Collaborator

ibgreen commented Jan 11, 2022

One of my long-standing roadmap ideas is to allow masks to be created from arbitrary sets of layers (not just polygons) and then applied to other layers.

This goes part of the way and I am excited to see it coming along.

@felixpalmer
Copy link
Collaborator Author

What happens if you need to apply the same mask to multiple layers? Is it more efficient to define the mask with a polygon layer?

Good point. Currently it will work, but it is wasteful as the FBO is created again for each layer. I see two options:

  • The polygon is defined when the extension is created, e.g. new MaskExtension({polygon})
  • Rather than having the maskPolygon prop we have a maskPolygonLayer prop. The FBO is then stored as state on this layer and when the layer is used for masking elsewhere the FBO is reused

@ibgreen I agree it would be really neat to allow masks from arbitrary layers and indeed this approach is pretty close. For this feature I would like to restrict to polygons only as they are the major use-case and I worry that allowing arbitrary layers would expand the scope too much right now.

@alasarr
Copy link
Contributor

alasarr commented Jan 12, 2022

I prefer the option of maskPolygonLayer, I think it fits better with state updates (if the mask is updated).

I'd name it maskLayer and limit right now to polygons. That way we can easily increase the support to other types of layers as @ibgreen is suggesting.

@alasarr
Copy link
Contributor

alasarr commented Jan 12, 2022

I mean to support other types of layers in the future not in this first version

@padawannn
Copy link
Collaborator

We've done some tests and it works really fast. These are some problems we have found.

  • We get an error every time that a layer or sublayer is destroyed.
  • It doesn't seem to work with MVT layers.
  • It doesn't seem to work with Google maps.
  • It doesn't seem to work with Heatmap layer. Maybe the problem is this error: ERROR :GL_INVALID_OPERATION : glDrawArrays: Source and destination textures of the draw are the same.

On the other hand, works really well with GeoJsonLayer, ScatterplotLayer and AggregationLayer

@Pessimistress
Copy link
Collaborator

For this feature I would like to restrict to polygons only as they are the major use-case and I worry that allowing arbitrary layers would expand the scope too much right now.

I think it's perfectly fine to support only polygons in the initial release, though we should design the API with the eventual full-featured extension in mind. Having a stable API that incrementally expands its capabilities is better than introducing new APIs frequently.

To work off of @alasarr 's idea, maybe something like:

layers: [
  new MaskLayer({
    id: 'mask',
    type: SolidPolygonLayer,
    data: <polygon_data>
  }),
  new ArcLayer({
    id: 'links',
    extensions: new MaskExtension({maskId: 'mask'})
  }),
  new ScatterplotLayer({
    id: 'nodes',
    extensions: new MaskExtension({maskId: 'mask'})
  }),
]

@alasarr
Copy link
Contributor

alasarr commented Jan 14, 2022

Makes sense for me

@felixpalmer
Copy link
Collaborator Author

Support the idea to have a future proof API. I was imagining something more like:

const maskLayer = new SolidPolygonLayer({
  id: 'mask',
  data: <polygon_data>
});
layers: [
  new ArcLayer({
    id: 'links',
    extensions: new MaskExtension(),
    maskLayer
  }),
  new ScatterplotLayer({
    id: 'nodes',
    extensions: new MaskExtension(),
    maskLayer
  }),
]

Having maskLayer as a prop feels more natural to me. Is there any technical reason to do the linking via a maskId instead?

Regarding placing the maskLayer in the set of standard deck layers: I can see how it would help as we could rely on the standard render cycle to trigger redraws, but often we will not want to actually draw the mask layer and it seems a bit clumsy to have to hide it in some form.

@ibgreen
Copy link
Collaborator

ibgreen commented Jan 14, 2022

FWIW, I always envisioned the masking feature to support an id. This would allow masks to be used in completely declarative environments, like @deck.gl/json (which is one of my "litmus tests" for new deck.gl API proposals).

Perhaps we could offer an overloaded prop mask? The user could supply

  • a Texture containing a "manually created" mask of the proper size and format
  • or a layer id
  • or possibly a layer.

Worth keeping in mind:

  • The deck.gl API has never encouraged users to hold on to and manipulate layers instances. Layers were conceived to be used to declaratively describe a visualization.
  • That said, we are not functional purists, we do want the API to evolve, make it easy to use for "imperative" programmers etc, so this type of proposals are absolutely on the table.
  • Regardless, every precedent we set has an impact on our developers and future feature design.

@alasarr
Copy link
Contributor

alasarr commented Jan 14, 2022

@ibgreen is this your idea? Just to be sure we're following you:

const maskLayer =  new SolidPolygonLayer({
    id: 'mask',
    data: <polygon_data>
  });

layers: [
  maskLayer,
  new ArcLayer({
    id: 'links',
    extensions: new MaskExtension(),
    mask: maskLayer
  }),
  new ScatterplotLayer({
    id: 'nodes',
    extensions: new MaskExtension(),
    mask: 'mask'
  }),
]

@Pessimistress why do you want to create the new MaskLayer? Is this because you want to define a common interface for the mask? For example maskLayer.getFBO()

@ibgreen
Copy link
Collaborator

ibgreen commented Jan 14, 2022

No that doesn't solve for the fully declarative case (JSON etc). I would imagine a single list of layers where some layers are marked as masks:

layers: [
  new SolidPolygonLayer({
    id: 'mask',
    mask: true,
    data: <polygon_data>
  });,
  new ArcLayer({
    id: 'links',
    extensions: new MaskExtension(),
    mask: maskLayer
  }),
  new ScatterplotLayer({
    id: 'nodes',
    extensions: new MaskExtension(),
    mask: 'mask'
  }),
]

While I am not sure if extensions are currently supported in deck.gl/json, this would set the stage for a JSON description:

  "layers": [
  {
    "type": "SolidPolygonLayer",
    "id": "mask",
    "mask": true,
    "data": []
  },
  {
    "type": "ArcLayer",
    "id": "links",
    "extensions": ["mask"],
    "mask": maskLayer
  },
  {
  "type": "ScatterplotLayer",
    "id": "nodes",
    "extensions": ["mask"],
    "mask": "mask"
  }
]

@Pessimistress
Copy link
Collaborator

why do you want to create the new MaskLayer?

Without changing how the core functions, a layer must be in the layers list to participate in the update cycle (prop diff, lifecycle method invocations, etc). But then if we just define a plain layer like in @ibgreen's example, it would be drawn to the render buffer. It's against our design to have the prop value of one layer to affect the behavior of another.

So one solution is to wrap the mask with a new layer class MaskLayer whose behavior we can control.

We can also add a parent layer for both the mask and the masked:

layers: [
  // layer instances that are not affected
  new GeoJsonLayer(),

  // mask and masked
  new MaskGroup({
    maskLayer: new SolidPolygonLayer(),
    layers: [
      new ScatterplotLayer()
    ]
  })
]

An alternative is to create a MaskEffect which is global. It is not supported right now but we can allow effects to filter the layers list before they are rendered.

@ibgreen
Copy link
Collaborator

ibgreen commented Jan 14, 2022

In my proposal I added a prop mask: true that was intended to indicate that this layer does not render to the target framebuffer.

 "layers": [
  {
    "type": "SolidPolygonLayer",
    "id": "mask",
    "mask": true,
    "data": []
  },

I actually wrote an RFC on layer groups a long time ago, it may be outdated but it did explore some of these idea. I hesitated to bring it up due to the scope creep.

It's against our design to have the prop value of one layer to affect the behavior of another.

I want to agree but won't that be hard to avoid in this case as clipping against the output of another layer is kind of the whole point?

@Pessimistress
Copy link
Collaborator

In my proposal I added a prop mask: true that was intended to indicate that this layer does not render to the target framebuffer.

Are you proposing that we add a mask prop to the base layer class and use it as a generic filter mechanism just like visible? Are we going to add more props like this for another use case in the future?

@ibgreen
Copy link
Collaborator

ibgreen commented Jan 14, 2022

Are you proposing that we add a mask prop to the base layer class and use it as a generic filter mechanism just like visible?

OK I see your point. I suppose it depends on how fundamental we feel that clipping is. One indication is that stencil operations are built into all low-level GPU apis.

Are we going to add more props like this for another use case in the future?

Good question. To avoid a prop explosion one could generalize it to say operation: 'mask' instead of mask: true.

@felixpalmer
Copy link
Collaborator Author

Thanks for the input. I'm on board with the mask id approach, to support the declarative use-case. I'm going to have a go at implementing the API as follows:

layers: [
  new SolidPolygonLayer({
    id: 'mask',
    operation: 'mask',
    data: <polygon_data>
  }),
  new ArcLayer({
    id: 'links',
    extensions: new MaskExtension(),
    maskId: 'mask'
  }),
  new ScatterplotLayer({
    id: 'nodes',
    extensions: new MaskExtension(),
    maskId: 'mask'
  }),
]

Some thoughts on the above discussion:

  • I wouldn't overload the maskId prop. Passing a texture is complex as you need to georeference it and as the camera zooms in it will pixelate. Permitting only an layer id means we can enforce that the layer is part of the deck.gl update cycle.
  • I prefer operation: 'mask' over mask: true. It reduces the number of props, is futureproof and would allow compound values (like we have for pointType), e.g. 'mask+draw' would allow the layer to be drawn normally and as mask without needing to duplicate.
  • I'm not strongly against the 'MaskLayer' approach, just find the above approach cleaner

@alasarr
Copy link
Contributor

alasarr commented Jan 17, 2022

Thanks a lot to everyone for giving the feedback here, I think we're getting a really good approach! I also like the idea of operation: 'mask', if the changes in the core are not so hard, I'd say that I prefer it to avoid adding a new layer.

@felixpalmer can't wait to see this implementation 😄

@felixpalmer
Copy link
Collaborator Author

PR has been updated: #6554

@felixpalmer
Copy link
Collaborator Author

felixpalmer commented Jan 19, 2022

Just to update on the API: Following input from @Pessimistress I have modified the approach so that the masking is implemented as an Effect, rather than an Extension. Thus the API becomes:

const maskEffect = new MaskEffect();

new Deck({
  effects: [maskEffect],
  layers: [
    new SolidPolygonLayer({
      id: 'mask',
      operation: 'mask',
      data: <polygon_data>
    }),
    new ArcLayer({
      maskId: 'mask',
      ...
    }),
    new ScatterplotLayer({
      maskId: 'mask',
      ...
    })
  ]
});

@felixpalmer
Copy link
Collaborator Author

Another update on the API: The MaskEffect is now internal and mask layers are marked by setting operation: OPERATION.MASK on a layer. Layers that require masking need to add the MaskExtension. Thus the API becomes:

import {OPERATION} from '@deck.gl/core';
import {MaskExtension} from '@deck.gl/extensions';

const maskExtension = new MaskExtension();

new Deck({
  layers: [
    new SolidPolygonLayer({
      id: 'mask',
      operation: OPERATION.MASK,
      data: <polygon_data>
    }),
    new ArcLayer({
      ...
      extensions: [maskExtension], // Enables masking and adds support for props below
      maskEnabled: true | false // Optional (defaults to true),
      maskByInstance: true | false // Optional (inferred by layer data)
    }),
    new ScatterplotLayer({
      ...
      extensions: [maskExtension]
    })
  ]
});

@ibgreen
Copy link
Collaborator

ibgreen commented Jan 26, 2022

Nit: I think it is a bit inconvenient to add a new export OPERATION. Importing an additional top-level symbol is rather inconvenient and it doesn't really add much value. I would just accept string values, i.e. operation: 'mask'.

Once we move to typescript, we can just type it as

type LayerProps = {
  operation?: 'mask' | '...' | '...';
}

FIWI, this is how I am designing the new luma.gl v9 API.

@andresramosp
Copy link

andresramosp commented Aug 14, 2022

Hello @felixpalmer ,

I am trying to use the Mask Extension with a HeatmapLayer, but it does not seem to work. I recently opened an issue:

#7184

Can you please confirm if the extension is meant to work with HeatmapLayer?

Thanks
Andrés

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

No branches or pull requests

6 participants