Projection mode changer #73

tischi · 2020-01-18T18:18:00Z

No description provided.

tischi · 2020-01-18T18:31:10Z

I mainly implemented methods to make the ProjectionModeChanger work: https://github.com/bigdataviewer/bigdataviewer-playground/blob/projectionModeChanger/src/test/src/sc/fiji/bdvpg/bdv/ProjectionModeChangerDemo.java#L14

Along the way, as discussed, several other things were touched, e.g. one interface was removed.

The naming conventions are not consistent, see here: #72

I suggest, we fix this with another PR.

NicoKiaru · 2020-01-19T15:56:50Z

src/main/java/sc/fiji/bdvpg/services/ISourceAndConverterService.java

@@ -96,6 +96,6 @@
    /**
     * Finds the corresponding registered sac for a source.
     */
-    SourceAndConverter getSourceAndConverter( Source source );
+    SourceAndConverter getSourceAndConverterFromSource( Source source );


Is this function new ? I don't remember where it comes from, but since you can have multiple SourceAndConverter for an identical Source, we should remove it and keep only the function getSourceAndConvertersOfSource.

cf COmment, this should return a List

NicoKiaru · 2020-01-19T17:21:45Z

Should I review it ? Is it ready ?

tischi · 2020-01-19T17:52:19Z

Should I review it ? Is it ready ?

Yes, please. I think we could merge it.

NicoKiaru · 2020-01-19T17:54:31Z

OK, I start the review now

NicoKiaru

We should address the problem between linking Source and SourceAndConverter

src/main/java/sc/fiji/bdvpg/bdv/projector/AccumulateMixedProjectorARGB.java

NicoKiaru · 2020-01-19T18:06:51Z

src/main/java/sc/fiji/bdvpg/bdv/projector/AccumulateMixedProjectorARGB.java

+			if ( source instanceof TransformedSource )
+				source = ( ( TransformedSource ) source ).getWrappedSource();
+
+			SourceAndConverter sac = SourceAndConverterServices.getSourceAndConverterService().getSourceAndConverterFromSource( source );


This could fail because a Source can have different converters and thus exists in different flavors. One Source -> 2 SourceAndConverters, one with a gray LUT and one with a completely different LUT, for instance.

We have to think about that, but that's a major issue. I had to deal with this in the remove(SourceAndConverter, BdvHandle) function from the display service, you should have a look at it and then let's rediscuss this. Maybe there's no better way...

Agreed! See conversations below.

src/main/java/sc/fiji/bdvpg/bdv/projector/AccumulateMixedProjectorARGB.java

NicoKiaru · 2020-01-19T18:17:53Z

src/main/java/sc/fiji/bdvpg/bdv/projector/Projection.java

-public class ProjectionTypes
+public class Projection
 {
+	public static final String PROJECTION_MODE = "Projection Mode";
+	public static final String PROJECTION_MODE_SUM = "Sum";
+	public static final String PROJECTION_MODE_AVG = "Avg";
+
 	public static final String MIXED_PROJECTOR = "Mixed Projector";
 	public static final String SUM_PROJECTOR = "Sum Projector";


cf comment above, maybe change this to int to fasten the computation

Ok. I could do this, but long term I think an enum would be the cleanest.
The only reason I was choosing String constants was because this is the only thing that works in Command choices. Thus, using ints not would make it more complicated because we have to implement the constants twice.

Ok, I'll investigate the performance and report back

src/main/java/sc/fiji/bdvpg/scijava/services/SourceAndConverterBdvDisplayService.java

NicoKiaru · 2020-01-19T18:31:12Z

src/main/java/sc/fiji/bdvpg/scijava/services/SourceAndConverterService.java

+    @Override
+    public SourceAndConverter getSourceAndConverterFromSource( Source source )
+    {
+        final List< SourceAndConverter > sacs = getSourceAndConverters();
+
+        for ( SourceAndConverter sac : sacs )
+            if ( sac.getSpimSource().equals( source ) )
+                return sac;
+
+        // Try again, with unwrapping; (TODO: do we need this?)
+        if ( source instanceof TransformedSource )
+            source = ( ( TransformedSource ) source ).getWrappedSource();
+
+
+        for ( SourceAndConverter sac : sacs )
+            if ( sac.getSpimSource().equals( source ) )
+                return sac;
+
+        return null;
+    }


This part I have major concerns. First part :

a Source can have muliply linked SourceAndConverter

a TransformedSource is different from a Source, so we should be able to set different projectors and properties if we deal with a Source or one of its transformations

Yes, I had the same problem. I wrote a some text about it below, which you probably did not see. I paste it here again:

The AccumulateProjectorFactory in bdv work on a list of Source and not SourceAndConverter, which is quite unfortunate for us, because I need to get the metadata (projectionMode) for each Source. I thus added a function to our SourceAndConverterService that returns the SourceAndConverter for a Source. I am now very confused, because in our framework (e.g. due to SourceAndConverterDuplicator) there could be multiple SourceAndConverter for the same Source java object, in principle with different metadata. Currently the function I added (i.e., getSourceAndConverterFromSource) simply returns the first matching SourceAndConverter.

I am not sure how to handle this properly, i.e. how to figure out to which SourceAndConverter the Source belongs.

Do you have some ideas?

Question that come to my mind is:

Do we really need the SourceAndConverterDuplicator or can we avoid having this? I checked for its usages and it is only used by the SourcesDuplicatorCommand. As the code does not need it, do your users need it or could you provide similar functionality differently?
If we really need the SourceAndConverterDuplicator, could we wrap the Source that we put into the new SourceAndConverter such that it becomes a different and unique java object? E.g., make a DuplicatedSource( source )?
What are the other places where we wrap a Source into a SourceAndConverter? Could we also there wrap the Source into a unique object?
Could/Should we enforce that the metadata for a Source is unique? That is, we would not allow different SourceAndConverter for the same Source to have different metadata?
If you do not know a solution I would ask on gitter.

Here's how I dealt with this issue in the remove from display :

bigdataviewer-playground/src/main/java/sc/fiji/bdvpg/scijava/services/BdvSourceAndConverterDisplayService.java

Lines 249 to 256 in c4f9b34

/**

* A reflection forced access to ViewerState.removeSource(int index)

* protected void removeSource( final int index )

* is necessary because the SpimSource is not precise enough as a key : it can be displayed multiple times with different converters

* all following calls fail:

* bdvh.getViewerPanel().getState().removeSource();//.removeGroup(sg);//remove(index);//.removeSource(source.getSpimSource()); // TODO : Check!!

*/

removeSourceViaReflection(bdvh, index);

NicoKiaru · 2020-01-19T18:35:13Z

src/main/java/sc/fiji/bdvpg/services/ISourceAndConverterService.java

@@ -96,6 +96,6 @@
    /**
     * Finds the corresponding registered sac for a source.
     */
-    SourceAndConverter getSourceAndConverter( Source source );
+    SourceAndConverter getSourceAndConverterFromSource( Source source );


cf COmment, this should return a List

src/main/java/sc/fiji/bdvpg/scijava/services/SourceAndConverterBdvDisplayService.java

NicoKiaru · 2020-01-20T07:01:27Z

The link between Sourceand SourceAndConverter is done in the method createProjector of the class MultiResolutionRenderer.

With:

the function: bdvHandle.getViewerPanel().getState().getVisibleSourceIndices()
a reference to BdvHandle being used by the factory
an access to BdvHandleReferences of the display service

We should be able to reconstitute the sequence that led to the projector factory call, and retrieve from which SourceAndConverter a Source comes from.

Another option would is to get the first SourceAndConverter that comes (as it is currently but it will fail sometimes). But we could make this assumption for now and see if the new BigDataViewer state in bigdataviewer/bigdataviewer-core#68 allows us to find more easily the link between Source and SourceAndConverter

src/main/java/sc/fiji/bdvpg/bdv/projector/AccumulateMixedProjectorARGB.java

… for faster comoputation

…erface by adding the function to retrieve sacs from a source

NicoKiaru

Looks all ok

tischi · 2020-01-21T09:41:24Z

Great work!
Thanks a lot!

Implement SacProjectionModeChangerCommand

c2a24dd

tischi requested a review from NicoKiaru January 18, 2020 18:18

tischi added 2 commits January 18, 2020 19:20

Merge latest master

e28606f

Merge latest master

ec713cd

tischi added 2 commits January 18, 2020 19:40

Remove the SourceAndConverterDisplayService interface

b39780d

Use getSourceAndConverterFromSource in AccumulateMixedProjectorARGB

2d72754

tischi mentioned this pull request Jan 19, 2020

Add context menu behaviour #75

Merged

NicoKiaru reviewed Jan 19, 2020

View reviewed changes

NicoKiaru requested changes Jan 19, 2020

View reviewed changes

tischi commented Jan 20, 2020

View reviewed changes

src/main/java/sc/fiji/bdvpg/bdv/projector/AccumulateMixedProjectorARGB.java Show resolved Hide resolved

tischi and others added 6 commits January 20, 2020 08:23

Remove instance of TransformedSource

d19a88d

Add bdvHandle to AccumulateMixedProjectorARGB

df97e5d

merge master and solve conflicts

4ada17d

modifies the Service API to use only setter and getter

cdc40b4

replaces .equals by == in comparison : tests only the memory location…

b5e6efb

… for faster comoputation

solves issue finding sourceandconverter from source + changes the int…

3c4b5b5

…erface by adding the function to retrieve sacs from a source

NicoKiaru approved these changes Jan 21, 2020

View reviewed changes

revert failed refactoring

5056954

NicoKiaru merged commit 10463f3 into master Jan 21, 2020

NicoKiaru deleted the projectionModeChanger branch January 21, 2020 07:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Projection mode changer #73

Projection mode changer #73

tischi commented Jan 18, 2020

tischi commented Jan 18, 2020

NicoKiaru Jan 19, 2020

NicoKiaru Jan 19, 2020

NicoKiaru commented Jan 19, 2020

tischi commented Jan 19, 2020

NicoKiaru commented Jan 19, 2020

NicoKiaru left a comment

NicoKiaru Jan 19, 2020

tischi Jan 20, 2020

NicoKiaru Jan 19, 2020

tischi Jan 20, 2020 •

edited

Loading

NicoKiaru Jan 20, 2020

NicoKiaru Jan 19, 2020

tischi Jan 20, 2020

NicoKiaru Jan 20, 2020

NicoKiaru Jan 19, 2020

NicoKiaru commented Jan 20, 2020 •

edited

Loading

NicoKiaru left a comment

tischi commented Jan 21, 2020

	/**
	* A reflection forced access to ViewerState.removeSource(int index)
	* protected void removeSource( final int index )
	* is necessary because the SpimSource is not precise enough as a key : it can be displayed multiple times with different converters
	* all following calls fail:
	* bdvh.getViewerPanel().getState().removeSource();//.removeGroup(sg);//remove(index);//.removeSource(source.getSpimSource()); // TODO : Check!!
	*/
	removeSourceViaReflection(bdvh, index);

Projection mode changer #73

Projection mode changer #73

Conversation

tischi commented Jan 18, 2020

tischi commented Jan 18, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NicoKiaru commented Jan 19, 2020

tischi commented Jan 19, 2020

NicoKiaru commented Jan 19, 2020

NicoKiaru left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tischi Jan 20, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NicoKiaru commented Jan 20, 2020 • edited Loading

NicoKiaru left a comment

Choose a reason for hiding this comment

tischi commented Jan 21, 2020

tischi Jan 20, 2020 •

edited

Loading

NicoKiaru commented Jan 20, 2020 •

edited

Loading