Make layer alpha configurable and generalize Rgb/Grayscale layer handling #3670
Conversation
…yers in the same dataset, design tweaks for layer settings
There was a problem hiding this comment.
Awesome! That will be a really cool update 👍 I added some minor suggestions, but in general, it should be good to go!
Two additional comments:
- I'm wondering whether we should rename "alpha" to "opacity" in the UI. Probably, the majority will instantly understand what "alpha" means, but I find "opacity" quite a bit clearer.
- Can you rename the "Colors" header of the accordion settings to "Color Layers"?
| function getColorLayerNames() { | ||
| return getColorLayers(Store.getState().dataset).map(layer => sanitizeName(layer.name)); | ||
| } | ||
|
|
||
| class PlaneMaterialFactory extends AbstractPlaneMaterialFactory { | ||
| function getRgbLayerLookup(): { [string]: boolean } { |
There was a problem hiding this comment.
Maybe rename it to getIsRgbLayerLookup()? Sounds a bit weird, but I think it reflects better what this function does (I was a bit confused at first, what the function does).
| <div key={layerName}> | ||
| <Row> | ||
| <Col span={24}> | ||
| <Tag color={isRGB && "#1890ff"}>{isRGB ? "24-bit" : "8-bit"} Layer</Tag> {layerName} |
There was a problem hiding this comment.
Can the Tag be made non-clickable? Otherwise, it feels like something is broken, since nothing happens if you click it.
Also, I'd swap the order of tag and name, so that the name comes first. Then, it "acts as a headline" for the whole layer segment (maybe making it bold would make sense, too). The tag would be more of an addendum in that case.
| isLastLayer: boolean, | ||
| ) => { | ||
| const isRGB = isRgb(this.props.dataset, layerName); | ||
| // $FlowFixMe Object.entries returns mixed for Flow |
There was a problem hiding this comment.
Interesting that flow does not complain at the call side, since this function closely defines that a DatasetLayerConfiguration is needed 🤔
| @@ -106,7 +106,7 @@ | |||
| font-size: 14px; | |||
| } | |||
|
|
|||
| .settings-row { | |||
| .margin-bottom { | |||
| @@ -86,12 +86,14 @@ function SettingsReducer(state: OxalisState, action: Action): OxalisState { | |||
| (result, layer) => { | |||
| if (initialLayerSettings != null && initialLayerSettings[layer.name] != null) { | |||
| result[layer.name] = initialLayerSettings[layer.name]; | |||
| if (initialLayerSettings[layer.name].alpha == null) result[layer.name].alpha = 100; | |||
There was a problem hiding this comment.
Can we change the logic here so that each initialLayerSetting is merged with the default settings (lines 93 to 96), so that only the missing values are set? That way, we wouldn't handle alpha as a special case (for which the default value is defined twice at the moment).
abstract_plane_material_factorywhich was a legacy OOP artifactalphaproperty to the data layer configuration, which can be used to fade the different layers in and outcolor) and more importantly a dataset can now consist of a mix of rgb and grayscale layersIdeally, we should also avoid loading and prefetching data for layers whose alpha value is 0 (same as for the segmentation). To do that, I think it would be very beneficial to soften the specialized handling of the segmentation layer somewhat, which would make it easier to use the existing code for all layers. I'll do that in another PR (Issue: #3671) :)
URL of deployed dev instance (used for testing):
Steps to test:
Issues: