Improve WritablePolygon2D and WritablePolyline #35
Conversation
|
@ctrueden Thank you for your work on this (and cleaning up some of my tests)!
I think these methods should go in Aside from that though, I think these changes are wonderful and (pending a decision on the above) are good to merge. Thoughts @tpietzsch? |
|
Thanks for reviewing, @awalter17. I filed imglib/imglib2#211 to add the |
|
Rebased over the latest master. I also added a commit cleaning up the |
Implementations of this interface are always 2-dimensional.
It now lives in a shared Polyshape interface.
It now lives in a shared WritablePolyshape interface. And the addVertex(int, double[]) method is deprecated in favor of addVertex(int, RealLocalizable).
Now it repeatedly delegates to an expandMinMax method allowing to grow the bounds vertex by vertex. This will come in handy shortly for performance reasons.
When repeatedly calling addVertex, the bounds were being recomputed from the entire (growing) collection of vertices every time. Now, we only expand the bounds as needed based on the new vertex. See: https://gitter.im/imglib/imglib2?at=5ae1d3f32d0e228d7bb52ea7
This method allows to add a collection of vertices all at once. One big advantage of supporting this is performance: it avoids repeated reallocation of internal data structures, since it is known in advance how many vertices are intended to be added in the batch. For DefaultWritablePolygon2D, the x and y Trove-based fields needed extension to support one-shot right-shifting of the arrays to enable performant mass injection of new values. For DefaultWritablePolyline, the vertices field needed more specific typing on ArrayList to make use of its ensureCapacity method.
The order is (expected, actual) not vice versa. While at it, also call assertEquals(long, long) rather than assertEquals(double, double, double) for integer values.
The javadoc was promising specific subtypes would be returned, but the methods were not actually typed on those specific subtypes. Let's back off on those promises, and instead explain things in an interface-driven way: in general, the closed* methods give a ROI with CLOSED boundary type, while the open* methods give a ROI with OPEN boundary type. This commit does narrow the return type of kDTreeRealPointCollection and realPointSampleListRealPointCollection, since it is helpful (especially for the RealPointSampleListRealPointCollection) to have the properly typed object for subsequent API calls like addPoint.
a9d841b to
12b0ddc
Compare
|
The build on |
|
Fixed with 07f5716 |
This branch makes the following improvements:
Polygon2DandPolylinecommon API is now inherited fromPolyshapeinterface.WritablePolygon2DandWritablePolylinecommon API is now inherited fromWritablePolyshapeinterface. TheWritablePolygon2D#addVertex(int, double[])method is deprecated in favor ofWritablePolyshape#addVertex(int, RealLocalizable).DefaultWritablePolygon2D#addVertex,DefaultWritablePolyline#addVertexandDefaultWritableRealPointCollection#addPointnow update the bounds based on the new point only, avoiding an O(n) bounds rebuild. (RealPointSampleListWritableRealPointCollectiondid not suffer from this issue.)WritablePolyshapegained anaddVerticesmethod for adding many vertices at once. This bulk operation enables the default implementations to perform better, expanding their internal storage once as appropriate, rather than doing repeated incremental expansions.GeomMathsto compareLocalizableandRealLocalizablepositions. (Maybe these should go intonet.imglib2.util.Util? What do others think?)WritablePolygon2DTestcode has been improved a bit.Note that
WritableRealPointCollectionstill does not haveaddPointsorremovePointsmethods; I considered doing them as part of this PR, but I have no immediate use case, so I've held off for now.With these changes, it will be more feasible to pass
WritablePolyshapeobjects as outputs into which calculated points can be injected. In particular, it will become feasible to recast ImageJ Ops's geometry operations such asgeom.convexHull(Polygon2D)as computer ops rather than function ops.