mobie / mobie.github.io

1 stars 3 forks source link

Serialisation #31

Closed tischi closed 3 years ago

tischi commented 3 years ago

@constantinpape (cc @NicoKiaru)

I know, in the end it's my job, but I would be grateful for some input. Below is one important MoBIE class that contains all the information that is needed to visualize label mask images.

public class SegmentationDisplay extends SourceDisplay
{
    // Current JSON fields (probably several missing, e.g. defining the scatterPlotViewer state)
    private final String name;
    private final List< String > sources; // defines the sources to be shown
    private final double alpha; // property of coloringModelWrapper
    private final String lut; // property of coloringModelWrapper, which defines the Converter of the sourceAndConverter
    private final List< String > selectedSegmentIds; // property of selectionModel (needs SegmentAdapter)

    // The actual classes needed at runtime, which can change their state
        // currently marked transient, because I do not directly serialize them
    public transient SelectionModel< TableRowImageSegment > selectionModel;
    public transient ColoringModelWrapper< TableRowImageSegment > coloringModel;
    public transient List< SourceAndConverter< ? > > sourceAndConverters; // the converters make use of the coloringModel
    public transient ImageViewer< ? extends ImageSegment > imageViewer;
    public transient TableViewer< TableRowImageSegment > tableViewer; // needs the segments
    public transient ScatterPlotViewer< TableRowImageSegment > scatterPlotViewer; // needs the segments

        // Those classes cannot change their state, so we don't need to serialize them
    public transient List< TableRowImageSegment > segments; // need to be loaded from disk
    public transient SegmentAdapter< TableRowImageSegment > segmentAdapter; // not needed for serialisation

I don't know how complex it will be but I have a feeling now that we should get rid of the JSON fields and instead implement Serializers for all the classes; this would also naturally structure the JSON by adding fields like coloringModel a.s.o.

However(?), since some of those objects are quite heavy to instantiate this could mean that there will be a lot (if not all) of the MoBIE code being called from within those Serializers, because MoBIE is all about generating these views. Thus I am a bit confused about the best practice for doing this...

NicoKiaru commented 3 years ago

Not sure I can help...

Maybe it's obvious, there's no need to always write a serializer (in gson terms an Adapater implements both the serializer and deserializer). That's only needed when you want to control how it is created. This link about control of object type can be useful : https://github.com/google/gson/blob/master/extras/src/main/java/com/google/gson/typeadapters/RuntimeTypeAdapterFactory.java

tischi commented 3 years ago

Maybe few considerations, taking the coloringModel as an example:

  1. Do we feel more nesting in the JSON is a good or a bad thing from readability point of view?
    { "segmentationDisplay": 
    { "coloring" : { 
      "lut" : "glasbey", 
      "alpha" : 1.0 
    }, 
    "selection" : { 
       "selectedSegments": [...] 
    }
    }
    }

vs.

{ "segmentationDisplay": 
  { 
  "lut" : "glasbey",
  "alpha" : 1.0,
  "selectedSegments": [...]
  } 
}
  1. The coloringModel is in fact not much more than a Converter< RealType, ARGBType > and could potentially be serialized as such. @NicoKiaru do you have a way of serialization for this already? We would need support for special LUTs and also an alpha (blending) value. If @NicoKiaru needs to serialize such objects also for the playground then for compatibility reasons maybe we should try to use the same code, if possible.
NicoKiaru commented 3 years ago

So there's an extensibility mechanism for serialization in bdv playground. Maybe the serializer can live in mobie, and just be caught in bdv playground. We can discuss this. That leaves you the flexibility of putting the converter that you'd like, serialiazing it the way you want, and still being compatible with bdv playground.

constantinpape commented 3 years ago

Some feedback from my side:

Eagerness of serialization

there will be a lot (if not all) of the MoBIE code being called from within those Serializers, because MoBIE is all about generating these views. Thus I am a bit confused about the best practice for doing this...

I think this depends on the context of how you are using the objects that are deserialized. Is there a value of having a "not fully build" version of the object? As an example, let's take a segmentation that has a table directory. We currently have different "levels" of building the object representing this:

  1. Set the table folder to the path specified in the json (and potentially create a list of the available tables)
  2. Set the table folder and load the default table default.tsv
  3. Set the table folder and load all tables.

This represents different degrees of lazyness/eagerness. I think it's clear that 3 does not make sense, because you would load tables that might never be selected by the user. 1 and 2 would both make sense, and the choice depends on whether there is some advantage of having the intermediate state 1, maybe to do some validation of the view.

Nesting the view spec

Personally I would like to avoid nesting with just a single element, e.g.

{"selection": {"selectedSegmentIds": [...]}}`

for more complex objects with more than one associated field nesting is fine and in some cases even preferable, e.g.

{
"scatterPlot": {"show": true, "axes": [...]}
}

is I think better than

{
"showScatterPlot": false,
"scatterPlotAxes": [...]
}

As a general comment I would very much like to avoid serializing java class names in order to keep the spec language independent.

tischi commented 3 years ago

@K-Meech The serialization can be done in many ways and I am still not sure what is the best. I thus suggest that we do the bookmark creation as the very last thing after we have all other things sort of stable in the new version.

K-Meech commented 3 years ago

No worries @tischi - makes sense! Also, if there are other refactoring parts I can help with, then let me know. I'll have some time after the easter holidays.

constantinpape commented 3 years ago

I think all of this is outdated and was solved by the new view concept. Reopen if something here is still relevant.