-
Notifications
You must be signed in to change notification settings - Fork 24
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
Add "merge" blend mode #6936
Add "merge" blend mode #6936
Conversation
- and add ui to switch rendering modes
- Fix layer precedence order.
As discussed, the code now decides whether a color is valid by |
- fix uint24 rendering - fix rendering layers below if the the upper layer has no data yet - fix color and opacity setting in cover blend mode
…s into cover-blend-mode
This is now also outdated. The cover blend mode now applies Alpha_blending. This automatically discards colors with an invalid alpha value. |
@philippotto Do you want to take over the pr review? Or should I ask daniel? |
float mixed_alpha_factor = (1.0 - data_color.a) * layer_color.a; | ||
float mixed_alpha = mixed_alpha_factor + data_color.a; | ||
vec3 cover_color_rgb = data_color.a * data_color.rgb + mixed_alpha_factor * layer_color.rgb; | ||
// Catching edge case where mixed_alpha is 0.0 and therefore the cover_color would have nan values. | ||
float is_mixed_alpha_zero = float(mixed_alpha == 0.0); | ||
vec4 cover_color = vec4(cover_color_rgb / (mixed_alpha + is_mixed_alpha_zero), mixed_alpha); | ||
cover_color = mix(cover_color, vec4(0.0), is_mixed_alpha_zero); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@daniel-wer I diverged from the original approach with the if statement to guard against the division by zero. Instead I used a neat tick that was already used a few code lines above where a simple 1.0
is added to the potentially dangerous divisor in case the divisor (mixed_alpha
) is equal to zero.
I simply saw how it was handled in the code lines above and think that it is even easier to ready than the if expression.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, sadly I cannot comment the lines I mean in the comment above. The trick is already applied in the lines 218 - 228.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left some code feedback :) My biggest question would be about segmentation layers: Did you discuss that these should be handled separately with the cover-mode?
docs/tracing_ui.md
Outdated
@@ -116,6 +116,12 @@ Note, not all control/viewport settings are available in every annotation mode. | |||
#### Data Rendering | |||
- `Hardware Utilization`: Adjusts the quality level used for rendering data. Changing this setting influences how many data is downloaded from the server as well as how much pressure is put on the user's graphics card. Tune this value to your network connection and hardware power. After changing the setting, the page has to be refreshed. | |||
- `Loading Strategy`: You can choose between two different loading strategies. When using "best quality first" it will take a bit longer until you see data, because the highest quality is loaded. Alternatively, "Progressive quality" can be chosen which will improve the quality progressively while loading. As a result, initial data will be visible faster, but it will take more time until the best quality is shown. | |||
- `Blend Mode`: You can switch between two modes of blending the color layer. The default (Additive) simply sums up all color values of all visible layers. The cover mode renders all color layers on top of each other. Thus the top most layer covers the layers below. This blend mode is especially useful for datasets using multi modality layers. Here is an example for such a dataset by Bosch et al. [1]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it true that this only affects color layers? Segmentation layers are handled differently?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Segmentation layers are handled differently?
Right now, that's exactly the case. The segmentation layer color pattern is added on top of the color value computed for the color layers.
Imo including the segmentation layer in that blending calculation might make things more complicated because in case a person has a color layer with opacity 100%, this would mean that this person could not see the segmentation. As this is currently the case (one can always see the segmentation if there is one), this might be confusing. Additionally, including the segmentation layer in the blending might require more configuration changes from a user for a typical use case for the "cover" blend mode: First change the blend mode to cover, then activate / deactivate the layer the user wants to see and finally tweak the opacities for the color layers so that the segmentation is still visible in the desired way the user wants.
Leaving the segmentation layer out of the blend mode calculation potentially removes the last step of the steps described above.
But I am open for discussion on this. I'll try to schedule a small meeting for this :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update: We already discussed a solution:
- Keep the segmentation layer special right now.
- Add another pr that enabled editing the layer order
- Add another pr that enables setting the blend mode for every single layer including the segmentation layer. The default for segmentation layer will be cover, the default for color layers will be additive.
frontend/javascripts/oxalis/geometries/materials/plane_material_factory.ts
Outdated
Show resolved
Hide resolved
vec4 color = getMaybeFilteredColor(layerIndex, d_texture_width, packingDegree, worldPositionUVW, suppressBilinearFiltering, supportsPrecomputedBucketAddress); | ||
|
||
vec4 usedFallbackColor = vec4(0.0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the semantics of this var is a simple boolean, right? in that case, maybe define a struct with a vec4 and a bool?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh wow, I never knew that glsl has structs 🤯. Thanks for the input 🙏
|
||
); | ||
float used_fallback = maybe_filtered_color_and_used_fallback[1].r; | ||
// For uint24 the alpha channel is always 0.0 :/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this still true?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh sorry, forgot to remove this comment. This is no longer true thanks to the changes in texture_bucket_manager.ts
.
color_value = color_value * <%= name %>_alpha * <%= name %>_color; | ||
// Marking the color as invalid by setting alpha to 0.0 if the fallback color has been used | ||
// so that it does not cover other colors. | ||
vec4 layer_color = vec4(color_value, used_fallback == 1.0 ? 0.0 : maybe_filtered_color.a * <%= name %>_alpha); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think, I'd simply rename color_value
to layer_value
and then don't define a new variable (as it's not necessary to have both at the same time, I think).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem is that the calculates done with color_value
are only for the rgb channels and not for the alpha channel. But the blending needs a color with an alpha channel. Thus the calculations on the color are done with vec3 color_value
and from that a full vec4 layer_color
is calculated and then used for the blending calculation.
If I would remove the layer_color
and rename color_value to layer_value, I would still have to construct a vec4
combining the color_value
with the alpha channel in the blending methods (now extracted into own methods). So the construction of an additional vec4
cannot be avoided imo.
@@ -222,14 +226,33 @@ void main() { | |||
color_value = abs(color_value - <%= name %>_is_inverted); | |||
// Catch the case where max == min would causes a NaN value and use black as a fallback color. | |||
color_value = mix(color_value, vec3(0.0), is_max_and_min_equal); | |||
// Multiply with color and alpha for <%= name %> | |||
data_color += color_value * <%= name %>_alpha * <%= name %>_color; | |||
color_value = color_value * <%= name %>_alpha * <%= name %>_color; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MichaelBuessemeyer can you give this entire code block a bit of love? variable naming could be more consistent and I'd also hope that the code for the different blend mode could be extracted into helper functions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the different blend mode could be extracted into helper functions?
That is a very good suggestion: I added a new glsl module called blending and extracted the methods there. I also renamed all variable to snail case and all methods & struct names to camelCase. Sadly, the name is very inconsistent in the shaders and thus I couldn't find a clear pattern what naming scheme / style to use. I also discussed this shortly with daniel (the naming style)
I reviewed it now, but I'm not sure whether the two of you already discussed some details I'm missing now. Independent from that, I think, it would be good to show a new dev instance to the team to keep these in the loop who gave feedback last time. |
Co-authored-by: Philipp Otto <[email protected]>
…s into cover-blend-mode
- make clear that the blend mode only applies to color layers excluding segment layers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the feedback philipp. I think I covered everything. Let's wait for the CI and then ask for some further testing feedback. I hope everything should work now as expected.
docs/tracing_ui.md
Outdated
@@ -116,6 +116,12 @@ Note, not all control/viewport settings are available in every annotation mode. | |||
#### Data Rendering | |||
- `Hardware Utilization`: Adjusts the quality level used for rendering data. Changing this setting influences how many data is downloaded from the server as well as how much pressure is put on the user's graphics card. Tune this value to your network connection and hardware power. After changing the setting, the page has to be refreshed. | |||
- `Loading Strategy`: You can choose between two different loading strategies. When using "best quality first" it will take a bit longer until you see data, because the highest quality is loaded. Alternatively, "Progressive quality" can be chosen which will improve the quality progressively while loading. As a result, initial data will be visible faster, but it will take more time until the best quality is shown. | |||
- `Blend Mode`: You can switch between two modes of blending the color layer. The default (Additive) simply sums up all color values of all visible layers. The cover mode renders all color layers on top of each other. Thus the top most layer covers the layers below. This blend mode is especially useful for datasets using multi modality layers. Here is an example for such a dataset by Bosch et al. [1]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Segmentation layers are handled differently?
Right now, that's exactly the case. The segmentation layer color pattern is added on top of the color value computed for the color layers.
Imo including the segmentation layer in that blending calculation might make things more complicated because in case a person has a color layer with opacity 100%, this would mean that this person could not see the segmentation. As this is currently the case (one can always see the segmentation if there is one), this might be confusing. Additionally, including the segmentation layer in the blending might require more configuration changes from a user for a typical use case for the "cover" blend mode: First change the blend mode to cover, then activate / deactivate the layer the user wants to see and finally tweak the opacities for the color layers so that the segmentation is still visible in the desired way the user wants.
Leaving the segmentation layer out of the blend mode calculation potentially removes the last step of the steps described above.
But I am open for discussion on this. I'll try to schedule a small meeting for this :)
vec4 color = getMaybeFilteredColor(layerIndex, d_texture_width, packingDegree, worldPositionUVW, suppressBilinearFiltering, supportsPrecomputedBucketAddress); | ||
|
||
vec4 usedFallbackColor = vec4(0.0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh wow, I never knew that glsl has structs 🤯. Thanks for the input 🙏
|
||
); | ||
float used_fallback = maybe_filtered_color_and_used_fallback[1].r; | ||
// For uint24 the alpha channel is always 0.0 :/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh sorry, forgot to remove this comment. This is no longer true thanks to the changes in texture_bucket_manager.ts
.
@@ -222,14 +226,33 @@ void main() { | |||
color_value = abs(color_value - <%= name %>_is_inverted); | |||
// Catch the case where max == min would causes a NaN value and use black as a fallback color. | |||
color_value = mix(color_value, vec3(0.0), is_max_and_min_equal); | |||
// Multiply with color and alpha for <%= name %> | |||
data_color += color_value * <%= name %>_alpha * <%= name %>_color; | |||
color_value = color_value * <%= name %>_alpha * <%= name %>_color; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the different blend mode could be extracted into helper functions?
That is a very good suggestion: I added a new glsl module called blending and extracted the methods there. I also renamed all variable to snail case and all methods & struct names to camelCase. Sadly, the name is very inconsistent in the shaders and thus I couldn't find a clear pattern what naming scheme / style to use. I also discussed this shortly with daniel (the naming style)
color_value = color_value * <%= name %>_alpha * <%= name %>_color; | ||
// Marking the color as invalid by setting alpha to 0.0 if the fallback color has been used | ||
// so that it does not cover other colors. | ||
vec4 layer_color = vec4(color_value, used_fallback == 1.0 ? 0.0 : maybe_filtered_color.a * <%= name %>_alpha); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem is that the calculates done with color_value
are only for the rgb channels and not for the alpha channel. But the blending needs a color with an alpha channel. Thus the calculations on the color are done with vec3 color_value
and from that a full vec4 layer_color
is calculated and then used for the blending calculation.
If I would remove the layer_color
and rename color_value to layer_value, I would still have to construct a vec4
combining the color_value
with the alpha channel in the blending methods (now extracted into own methods). So the construction of an additional vec4
cannot be avoided imo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great to me :) Looking forward to the future iterations with layer ordering and layer-specific blend modes.
…ty-list-drawings * 'master' of github.com:scalableminds/webknossos: [Docs] Update embedded YouTube URLs (#7102) Show organization in dataset info tab (#7087) Added Tutorials to Docs (#7095) Combine both download modals into one (#7068) Add "merge" blend mode (#6936) Changed the spacing/width of VX reports runs selection (#7094) Allow dataset managers to see all workflow reports of their orga (#7081) Fix rectangle at 0,0,0 (#7088) Allow to disable automatic reloading of meshes during proofreading (#7076)
URL of deployed dev instance (used for testing):
TODOs
10.0
and but the uint24 case does not.packingDegree = 2
/byteCount = 2
is still untested. This is not included the data layers dataset.Steps to test:
Issues:
(Please delete unneeded items, merge only when none are left open)