Viewport with empty layer ids list shows all layers

[rubenthoms]

I would expect a viewport with an empty layer ids list to not show anything.

[w1nklr]

It is for convenience. If you display a single view (the usual case), you probably don't want to specify all the displayed layers on one side plus all the corresponding layer ids for the viewport...

[rubenthoms]

@w1nklr I understand the use case. However, in my opinion, sending in an empty list should result in showing no layers, as specified. Why can't layerIds be left undefined if one wants all layers to be displayed?

[w1nklr]

I don't know. I was not there at the initial design... I don't really see the use case needed to display nothing.

But if you have such an use case and if you can provide an easy way to display all the layers (and set that as DEFAULT_VIEWS), I don't object to a change.

BTW, this would be closer to Deck.gl approach: https://github.com/visgl/deck.gl/blob/4dd814805121ab70c16ea3b458593ecc92f92f1a/docs/api-reference/core/layer-manager.md?plain=1#L46

[rubenthoms]

We have a component where individual viewports can be added. To each of the latter custom layers can be added later on (in some sort of tree arrangement). When adding a layer to the first viewport, it becomes visible in all (empty) viewports although they should not have any children. We would be glad to have the possibility to temporarily have views without any layers.

[hkfb]

Agree, it makes sense to be able to show empty views, especially in the multi-viewport case.

It should be a minor change. Who will fix this (preferably with a story to demonstrate)?

[danielsantiago1002]

Hey @hkfb, I've worked on the issue and have a few questions before moving forward.

I found the part of the code that creates the displayed behavior:

https://github.com/equinor/webviz-subsurface-components/blob/master/typescript/packages/subsurface-viewer/src/components/Map.tsx#L829

(Changing the function's return value to "false" filters out the layer that is not found within the array of layers, The expected behavior for the issue solution is obtained.)

However, I found one possible Edge case that might be worth checking out:

• Currently, the user is not required to declare the "views" object. A default views object is used when one is not provided, as follows: https://github.com/equinor/webviz-subsurface-components/blob/master/typescript/packages/subsurface-viewer/src/components/Map.tsx#L103

With the proposed change to the behavior of the (layersIds) property of viewports, we would be creating a viewport without layers by default (in short, the user would be required to create the view object to obtain something other than a viewport without layers)

The question is:

• Should we add functionality to add a default view that includes all layers (only if the "views" object is undefined)? Or should we expect users to define the views object from now on? (There are breaking changes in the storybook examples.)

Thanks!

(In this issue we should also correct the examples that have viewport objects with layerIds = [ ], they should explicitly define the layer ids)

[hkfb]

• Should we add functionality to add a default view that includes all layers (only if the "views" object is undefined)? Or should we expect users to define the views object from now on? (There are breaking changes in the storybook examples.)

That's a good observation.

We should not require views to be defined or change the behaviour for when views is undefined. I think the current behavior makes sense and is not surprising, ie. falling back to a default singular view with all layers displayed.

However, when views is defined, we usually want to explicitly control view behavior, eg. views layout, visible layers, projection. In this case, as @rubenthoms outlines above, it's surprising that an empty layerIds array is interpreted as all layers.

(In this issue we should also correct the examples that have viewport objects with layerIds = [ ], they should explicitly define the layer ids)

@rubenthoms suggests above that leaving layerIds undefined would mean to show all layers. Could that be an option? Ie. in the stories where layerIds = [ ] we could just remove layerIds and get the same behaviour?

[danielsantiago1002]

I created a pull request to solve the issue, taking the feedback into account @hkfb. https://github.com/equinor/webviz-subsurface-components/pull/2655

I had to change some parts of the tests (I don't know if this is correct or if some new ones need to be done), also some logic related to a loader (it took into account all visible layers and not those that are incluided in the views).

I await your feedback or any further changes.

Thanks!