visgl / deck.gl

WebGL2 powered visualization framework
https://deck.gl
MIT License
12.06k stars 2.08k forks source link

[Feat] 'pixels'-like unit without depth scaling #7992

Open felixpalmer opened 1 year ago

felixpalmer commented 1 year ago

Target Use Case

The 'pixels' unit is frequently used to draw features with a fixed screen size. As per the documentation, a scaling factor is applied when the pitch is non-zero in other to provide a sense of depth. While for the mapping use-case (where the pitch is often constrained) this is valid, for other situations the effect is not desirable and confusing for users, especially when dealing with Point Clouds where the camera can often point at the horizon, resulting in huge features near the camera.

Screenshot 2023-07-10 at 11 03 22

The source of this behavior is the projection shader code which sets the gl_Position.w to a different value depending on the depth in clip space. The project_pixel_size_to_clipspace function doesn't take this into account and only applies the offset to gl_Position.xy

Proposal

It is possible to workaround this behavior by modifying the shader (see demo: https://jsfiddle.net/felixp/5kj497mw/)

class FixedSizeExtension extends LayerExtension {
  getShaders() {
    return {
      inject: {
        'vs:DECKGL_FILTER_SIZE': 'size *= 0.7 * gl_Position.w;'
      }
    };
  }
}

However this isn't very clean, and it would be nice if deck supported a unit which used pixels without the depth scaling.

I'm not sure what is the best name to use, as we want to keep 'pixels' back-compatible, perhaps sizeUnits: 'screen' or sizeUnits: 'static'?

mpearson commented 11 months ago

Really need this feature. Properties like radiusMinPixels are highly misleading since they have absolutely no effect on the min size when the camera is pitched down. An alternative to changing the unit type would be a simple depthScale flag on layers like ScatterPlotLayer or IconLayer, e.g.

const layer = new IconLayer({
   id: 'icon-layer',
   data: ...,
   getSize: d => d.size,
   sizeUnits: 'pixels',
   billboard: true,
   depthScale: false,
});

This seems like a relatively easy feature to implement on ScatterPlotLayer, IconLayer, and PointCloudLayer. If I fix it and open a PR, how likely is that to get merged?

felixpalmer commented 11 months ago

@mpearson if you could take a look at this that would be great, and could be added in the next major release which is coming soon. I'm happy to review and help get any PR merged.

I'm not in favor of adding depthScale as it overlaps with the sizeUnits prop though - also it is confusing in conjunction with sizeUnits: 'meters'. To get the PR merged we would need implement the shader changes in the three layers you mentioned as well as render tests demonstrating they work

mpearson commented 11 months ago

@felixpalmer I have to agree, it would be contradictory if units were set to meters but didn't scale with depth. I'm a little worried that the allowed units might be entangled with many other things in the codebase, and it's not possible to add an extra option in just one place. But I'll take a look sometime this week.

yaras-phoenix commented 4 months ago

But I'll take a look sometime this week.

Any chance you could take a look? Guess this case is still relevant.