[geos-devel] Exposing PrecisionModel, GeometryPrecisionReducer in the C API

[Sandro Santilli]

Sandro, others:
here's what I'm thinking to add as the single new signature
to the C-API for GEOS-3.6.0, what do you think ?

 /*
  * Set the geometry's precision, optionally rounding all its
  * coordinates to the precision grid (if it changes).
  *
  * @param gridSize size of the precision grid, or -1 for FLOATING
  *                 precision.
  * @param forceSnap specifies whether snapping to the precision grid
  *                  should be forced: -1 prevents snapping, which can
  *                  be used when the input is known to be already
  *                  rounded to the target grid; 0 only snaps if the
  *                  new precision grid is not equal or compatible with
  *                  the new precision grid; 1 always snaps.
  *
  */
 extern GEOSGeometry GEOS_DLL *GEOSGeom_setPrecision_r(
                                       GEOSContextHandle_t handle,
                                       double gridSize, int forceSnap);

Of course it can only work if the memory management for
GeometryFactory is automatic, as proposed in the PR
(which might be dangerous for any existing C++ client out there).

--strk;

On Thu, Oct 01, 2015 at 04:47:52PM +0200, Sandro Santilli wrote:
> So to followup,
> it is confirmed that the QGIS wrapper in its current incarnation
> is not useful for the purpose of changing the precision model of the
> operations.
> 
> Meanwhile I've worked on a reference-counting mechanism for
> GeometryFactory that would allow us to expose a single function
> to change a geometry's PrecisionModel (effectively it's reference
> "GeometryFactory") in a way that avoids memory leaks.
> 
> Here's the code: https://github.com/libgeos/libgeos/pull/52
> 
> Basically each GeometryFactory will be holding a count of Geometry
> objects referencing it. Every Geometry will signal its birth and death
> to it. When you don't need a GeometryFactory anymore you can request
> its auto-destruction and it'll stay alive only as long as any of its
> child is will also be alive.
> 
> Explicitly deleting a GeometryFactory before any of its child would
> become a coding error, in the current version of the code. Eventually,
> this may change to be more tolerant (at the cost of growing the size
> of a GeometryFactory to store all child pointers in a list).
> 
> With such kind of automatic memory management we'd be in a position
> to expose at the C-API level a single function to change a
> Geometry's precision. Such function would find or create an appropriate
> GeometryFactory, assign it to the geometry and mark it for
> auto-destruction. The calling code would have no need to ever know
> about the existanc eof such GeometryFactory object.
> 
> How would you feel about such a change ?
> 
> --strk;
> 
> On Wed, Sep 30, 2015 at 06:16:03PM +0200, Sandro Santilli wrote:
> > Sandro, I've been looking at the wrapper you added to QGIS
> > but from what I read it doesn't seem that the geometry coming out
> > of GEOSGeometryPrecisionReducer_reduce has an updated PrecisionModel,
> > which means overlay operations won't be using that new model.
> > 
> > Do you confirm ?
> > 
> > If that's the case, the wrapper doesn't do much more than a
> > grid-snapping (plus some topological fixes of the snapping),
> > basically exposing no real management of PrecisionModels...
> > 
> > Are there tests to verify that ?
> > 
> > REF: https://trac.osgeo.org/geos/ticket/713
> > 
> > --strk;
> > 
> > On Wed, Jan 14, 2015 at 10:38:23AM +0100, Sandro Santilli wrote:
> > > On Tue, Jan 13, 2015 at 11:21:34AM +0100, Sandro Mani wrote:
> > > > 
> > > > On 08.01.2015 13:08, Sandro Mani wrote:
> > > > >So to wrap up, things currently look as follows (which I actually
> > > > >think is quite neat!):
> > > > >
> > > > >
> > > > >/** GEOSContextHandle_t internally keeps a reference count, set to
> > > > >1 on creation
> > > > > * - *All* methods producing a GEOSGeom increase the reference count
> > > > > * - GEOSGeom_destroy decreses the GeometryFactory reference
> > > > >count, if 0, it destroys the context
> > > > > * - finishGEOS_r decreses the reference count, if 0, it destroys
> > > > >the context
> > > > > */
> > > > >
> > > > >/** GEOSGeom_clone() clones the geometry and applies
> > > > >PrecisionReducer::reduce
> > > > > *  if the PrecisionModel of the current context is different than
> > > > >the that of
> > > > > *  the passed geometry
> > > > > */
> > > > >
> > > > >/** Set context handle precision model **/
> > > > >void GEOSContextHandle_t
> > > > >GEOSContext_setPrecisionDouble(GEOSContextHandle_t* handle);
> > > > >void GEOSContextHandle_t
> > > > >GEOSContext_setPrecisionFixed(GEOSContextHandle_t* handle, double
> > > > >scale);
> > > >
> > > > So, what do you think? Worth pursuing this approach?
> > > 
> > > Sorry I'm not having much time to dedicate to this.
> > > I'm not very sure about the approach, to be honest.
> > > There are indeed 2 possible things one would want to do with PM:
> > > 
> > >  1. "advertise" the PM of a geometry, w/out rounding coords
> > > 
> > >  2. "set" the PM of a geometry, rounding coords
> > > 
> > > I'm not sure we ever want to allow for 1, to avoid wrong advertisement.
> > > The signature in the current proposal makes 2 clear for the cloning
> > > of a geometry, but we're still missing clear documentation about what
> > > happens from the constructors. Would/should they round ?
> > > 
> > > Example, what should happen here:
> > > 
> > >   GEOSContextHandle_t h = initGEOS_r(...);
> > >   GEOSCoordSequence *s = GEOSCoordSeq_create_r(h,2,2);
> > >   GEOSContext_setPrecisionDouble(h);
> > >   GEOSCoordSeq_setX_r(h, s, 0, 0);
> > >   GEOSCoordSeq_setY_r(h, s, 0, 0);
> > >   GEOSContext_setPrecisionFixed(h, 1);
> > >   GEOSCoordSeq_setX_r(h, s, 1, 0.2);
> > >   GEOSCoordSeq_setY_r(h, s, 1, 0.2);
> > > 
> > > Should the coordinate sequence contain 2 equal points after the above ?
> > > And what about WKB/WKT parsing ?
> > > 
> > > It would help to gather more comments from others.
> > > I've added Oliver and Sean in Cc as they took part of a precedent
> > > discussion about this:
> > > http://lists.osgeo.org/pipermail/geos-devel/2014-February/006780.html
> > > 
> > > --strk;