mobie / mobie-viewer-fiji

BSD 2-Clause "Simplified" License
32 stars 14 forks source link

Opacity displayed wrong #695

Closed martinschorb closed 2 years ago

martinschorb commented 2 years ago

Hi,

When I change the opacity of annotations to maximum, these turn white instead of keeping their colour.

martinschorb commented 2 years ago

This seems to be a general problem of opacity, also for images that it changes the display contrast instead of the actual opacity.

Screenshot 2022-04-29 at 12 11 47

martinschorb commented 2 years ago

also happens without an annotation present: Screenshot 2022-04-29 at 12 14 05

tischi commented 2 years ago

It is not really a bug, it has to do with our current definition of opacity (and having no good layering concept yet).

image image

It is just that if you add something very bright to something else very bright you get white.

Changing this is out-of-scope for now.

We could however try to come up with a better name instead of "opacity".

The code looks like this:

final int alpha = ARGBType.alpha( color.get() );
color.mul( alpha / 255.0 );
color.mul( opacity );

with

public void mul( final double c )
{
    final int value = get();
    set( rgba( red( value ) * c, green( value ) * c, blue( value ) * c, alpha( value ) * c ) );
}

So we are simply multiplying all RGB components with the opacity.

During rendering, every RGB that is >255 will be clipped to 255, thus it can become white.

I think "brightness" could be a better word, but we had B already assigned to the ContrastLimits ands C already assigened to Color, so I was running out of good letters....

Any suggestions (that do not involve developing a proper layering concept)?

Ideally I would just change the names (and button labels).

martinschorb commented 2 years ago

so we are in RGBA space, then alpha should be adjusted, not the RGB values.

Alpha of the current source (the one that is modified) needs to be multiplied with the opacity factor and all other alphas with (1-opacity).

martinschorb commented 2 years ago

or maybe even only adjusting alpha of the current image would be enough.

tischi commented 2 years ago

then alpha should be adjusted

In the end, alpha (A) is ignored by BDV, that's why we have to multiply it ourselves onto the RGB values at some point (I think we may be able to drag it a bit further along and only do something in the Projector, but I am not sure). I think the standard Projector in BDV ignores the alpha, thus better if we do something with it before it reaches that point.

martinschorb commented 2 years ago

then the code above seems good. It is just that also the other elements (sources and annotations) need to be multiplied by (1-c). That way the 255 will nor be exceeded and we won't run into the white-clipping situation. Is that feasible?

tischi commented 2 years ago

then the code above seems good. It is just that also the other elements (sources and annotations) need to be multiplied by (1-c). That way the 255 will nor be exceeded and we won't run into the white-clipping situation. Is that feasible?

I had a brief look and I think we could try that.

The place to do this is here: https://github.com/mobie/mobie-viewer-fiji/blob/2ac299231fa7e9c5fcd3baa01ffea36b1d45c9c4/src/main/java/org/embl/mobie/viewer/bdv/render/AccumulateOccludingProjectorARGB.java#L110

If you would be motivated to make some (pseudo-code) suggestions how to modify this would be great (I may not manage to still implement it today, but maybe next week).

The formulas that I have seen for this (1-c) logic did assume some order to the images though. If we could come up with something that did not do this would be good.

tischi commented 2 years ago

I created a branch and draft PR for this: https://github.com/mobie/mobie-viewer-fiji/pull/697

tischi commented 2 years ago

First good news, it looks like we can pull the alpha through to the Projector and apply it there (which is much nicer than the solution I had so far, where I applied it immediately):

        for ( int sourceIndex = 0; sourceIndex < accesses.length; sourceIndex++ )
        {
            final int argb = argbs[ sourceIndex ];
            final int a = ARGBType.alpha( argb );
            if ( a == 0 ) continue;
            if ( isOccluded( argbs, occludedBy.get( sourceIndex ) ) ) continue;

            final int r = ARGBType.red( argb );
            final int g = ARGBType.green( argb );
            final int b = ARGBType.blue( argb );

            final double alpha = a / 255.0;
            aAccu += a; // does this make sense??
            rAccu += r * alpha;
            gAccu += g * alpha;
            bAccu += b * alpha;
        }

Now we just would have to add some math to dim the other images accordingly, i.e. implement your (1-c) idea.

If you think about this, please keep in mind that whatever we implement must also work for fluorescence images, where the expectation is that the the intensity do simply add up.

tischi commented 2 years ago

Here are some formulas for inspiration: https://en.wikipedia.org/wiki/Alpha_compositing As mentioned, I think the challenge will be to find something that generates the expected appearance for the fluorescence channels (without treating fluorescence differently, which would bring us back to defining more blendingMode metadata, which we had at some point and then removed again to reduce the complexity).

martinschorb commented 2 years ago

I think the key for us is occluding:

in general, we need to support two different blending modes.

additive:

here alpha and also opacity can be entirely ignored (as in vanilla BDV). You just sum the RGB values until saturation.

alpha:

This will add opacity to the occluding case. (Our present "occluding" basically means ) So instead of replacing all underlying sources with the occluding source, you execute an alpha composition for each channel.

with C being the output blended value (rAccu,...) after adding each source and I the individual intensity (r, g, b). Annotations should then be occluding as well. The only annoying thing that we need to consider here is related to the "zeroValue" https://github.com/bigdataviewer/bigdataviewer-playground/issues/250

so here we need to introduce a catch for zero-value pixels to become additive instead. Otherwise Annotations (or any sources with a lot of zeros) would entirely overwrite everything.

martinschorb commented 2 years ago

in fact the zero-value issue is even easier, because at every pixel there should be a value for a. So in the additive case we just need to set that to zero: if r+g+b ==0: a = 0

tischi commented 2 years ago

Looking promising:

image image

But there seem to be now two separate functions of the alpha of one source: If the source is occluding, we do * (1-alpha) of the previous sources, but if the source is not occluding we should do * alpha of the source itself? Is that correct?

martinschorb commented 2 years ago

I think for a non-occluding = additive source, opacity should just multiply the intensity values that will be added and not touch the underlying sources.

martinschorb commented 2 years ago

so exactly as you have it


                        rAccu += r * alpha;
          gAccu += g * alpha;
          bAccu += b * alpha;
tischi commented 2 years ago

OK, this seems to work quite well:

            final double occludingAlpha = occludingAlpha( argbs, occludedBy.get( sourceIndex ) ) / 255.0;

            if ( ! isOccluding.get( sourceIndex ) )
            {
                rAccu += r * alpha * ( 1 - occludingAlpha );
                gAccu += g * alpha * ( 1 - occludingAlpha );
                bAccu += b * alpha * ( 1 - occludingAlpha );
            }
            else
            {
                rAccu += r * ( 1 - occludingAlpha );
                gAccu += g * ( 1 - occludingAlpha );
                bAccu += b * ( 1 - occludingAlpha );
            }

But, for the LM tomo, which is both occluding itself and also occludedBy the HM tomo, it behaves weird:

image

The LM does indeed become more transparent, but somehow also the HM becomes brighter.... Not sure yet why...

martinschorb commented 2 years ago

So the only thing left do check is zero-values...?

tischi commented 2 years ago

Yes, running in that right now in fact... Would you have time again to come?

martinschorb commented 2 years ago

unterwegs...

tischi commented 2 years ago

@constantinpape

BlendingMode blendingMode = display.getBlendingMode();
// https://github.com/mobie/mobie-viewer-fiji/issues/695
// we decided to make all label source occluding
blendingMode = BlendingMode.SumOccluding;

@martinschorb Were working on the blending and find that all annotations and segmentation should probably always have the BlendingMode.SumOccluding.

Should we:

a) remove the ability to adjust this from the spec? b) change the blending mode to SumOccluding in all our dataset.json?

By the way: it looks much better now, because the annotations maintain their color:

Opacity 0.5

image

Opacity 1.0

image
constantinpape commented 2 years ago

a) remove the ability to adjust this from the spec? b) change the blending mode to SumOccluding in all our dataset.json?

Do you think there is any reason that this should ever be changed to sum for segmentations? If not, then I would go with a) and remove it from the spec for segmentationDisplay.

For sourceAnnotationDisplay it's currently not in the spec.

tischi commented 2 years ago

I'd be for removing it from the spec.

constantinpape commented 2 years ago

I'd be for removing it from the spec.

Done.

constantinpape commented 2 years ago

This should be fixed with the current MoBIE-beta. @martinschorb could you check that it actually works and then close this issue?

martinschorb commented 2 years ago

Confirm it works in beta.