search

imglib / imglib2-roi

Regions of interest (ROIs) and labelings for ImgLib2
Other
8 stars 8 forks source link

RealPointCollection and GeomMasks Fixes #34

Open awalter17 opened 6 years ago

awalter17 commented 6 years ago

Hello,

@tpietzsch and I were discussing some oddities about RealPointCollection and the corresponding methods for them in GeomMasks.

  1. DefaultWritableRealPointCollection's HashMap constructor

    Realistically, someone probably won't have a HashMap of these specifications laying around. So the method in GeomMasks probably isn't very useful, and the same can be said for the constructor itself. Therefore, it is probably best to switch this constructor to protected visibility.

  2. Many realPointCollection(...) methods in GeomMasks

    Currently, there's three different types of RealPointCollections. And right now GeomMasks has a method for every constructor. This is overkill. The intention of GeomMasks is to provide a convenient way for creating ROIs, and a number of the realPointCollection(...) methods are more special cases and probably won't be used often.

    To simplify this we could reduce the number of methods to three.

    1. WritableRealPointCollection< L > realPointCollection( final Collection< L > points ) which would return a DefaultWritableRealPointCollection
    2. WritableRealPointCollection< L > realPointCollection( final RealPointSampleList< L > points ) which would return a RealPointSampleListWritableRealPointCollection
    3. RealPointCollection< L > realPointCollection( final KDTree< L > tree ) which would return a KDTreeRealPointCollection

    There is an outstanding issue with this though. Should the realPointCollection(...) method using a KDTree be named differently, in order to make it obvious that the RealPointCollection returned is read only? If so, what should it be named? immutableRealPointCollection, readOnlyRealPointCollection, etc.?

  3. Update RealPointSampleListWritableRealPointCollection Javadoc

    This javadoc should specify that the underlying RealPointSampleList is mutated when the RealPointCollection is mutated.

I believe that is everything. Please feel free to let me know your thoughts on this!

ctrueden commented 6 years ago

Should the realPointCollection(...) method using a KDTree be named differently

I vote to leave the name the same. The return type is different, which is enough of a hint. People will discover very quickly that they cannot mutate it, when their code doesn't compile. 😉

tpietzsch commented 6 years ago

Hmmm, then should we add RealPointCollection< L > immutableRealPointCollection( final Collection< L > points ) to build a KDTree from the given points? Or do we expect users to build their own?