Opened 6 years ago
Closed 5 years ago
#896 closed defect (fixed)
CAPI functions do not preserve SRID
Reported by: | dbaston | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | 3.8.0 |
Component: | Default | Version: | 3.6.2 |
Severity: | Unassigned | Keywords: | |
Cc: |
Description
For example, the following test fails:
void object::test<11>() { geom1_ = GEOSGeomFromWKT("LINESTRING(0 0, 10 0)"); GEOSSetSRID(geom1_, 32145); geom2_ = GEOSOffsetCurve(geom1_, 2, 0, GEOSBUF_JOIN_ROUND, 2); ensure_equals( GEOSGetSRID(geom1_), GEOSGetSRID(geom2_) ); }
It's obvious that SRID could be preserved here. For binary operations, I'd say that it's not the job of GEOS to check SRID consistency, and that the result could have the SRID of the first argument.
Any thoughts about this?
Change History (15)
comment:1 by , 6 years ago
Milestone: | 3.6.3 → 3.7.0 |
---|
comment:2 by , 6 years ago
Did you check what do Intersection/Union do ? I suspect OffsetCurve is just behind as being original code (rather than direct JTS port)
comment:3 by , 6 years ago
No, I didn't check intersection or union. I started writing unit tests for this, and then it occurred to me that maybe this wasn't considered a bug. If there's agreement that it's a bug, I can go ahead with tests and fixes for all functions.
comment:4 by , 6 years ago
It looks like the problem is unique to the C API, and affects all functions that produce a geometry as an output.
When a GEOS Geometry is created, it is given the SRID of its GeometryFactory. However, the C API does not use the concept of a GeometryFactory and provides no access to it. When the C API function GEOSSetSRID is called, it sets the SRID of an individual geometry, but leaves the GeometryFactory untouched.
Any objections to resolving this at the C API level, by setting the SRID of created geometries to match their inputs?
comment:5 by , 6 years ago
I've got no objections. I don't even have objections if you want to back port to 3.6, 3.5 as it does sound like a bug.
comment:6 by , 6 years ago
I don't think GeometryFactory should be affected by GEOSGeom_setSRID. JTS also doesn't set SRID of output geometry from operations, I wonder what Martin Davis thinks about this.
I'm +0 with adding SRID propagation in C-API (as I guess as of now all clients are already doing this in their converters)
comment:7 by , 6 years ago
So it turns out Geometry#setSRID is supposed to be only used in exceptional cases, for backward compatibility: https://github.com/locationtech/jts/blob/master/modules/core/src/main/java/org/locationtech/jts/geom/Geometry.java#L264
Rather, a user should indeed set the SRID in the factory, which we're not doing from C-API.
Now that we DO have garbage-collected factories (for precision model) we might start doing something about SRID too. Sounds like a wider plan is needed though.
comment:8 by , 6 years ago
Hi strk, I probably expressed myself poorly. I don't think the C API should modify the GeometryFactory.
Because it is not possible to manipulate a GeometryFactory in the C API, I think that the C API should set the SRID of the output geometries (after they are created) to match the SRID of the inputs.
I don't think we should try to retrofit GeometryFactories into the C API.
comment:9 by , 6 years ago
Summary: | Many (all?) GEOS operations do not preserve SRID → CAPI functions do not preserve SRID |
---|
comment:11 by , 6 years ago
To attempt to further clarify, I am proposing that C API functions be modified like this (from GEOSUnaryUnion_r
):
GeomPtr g3 ( g->Union() ); g3->setSRID( g->getSRID() ); // proposed modification return g3.release();
comment:12 by , 6 years ago
Milestone: | 3.7.0 → 3.8.0 |
---|
comment:13 by , 5 years ago
Thoughts, @dbaston? Now or push?
comment:14 by , 5 years ago
I'm less and less understanding the use case for GEOS to carry a 4-byte SRID on every geometry (actually 8 bytes I think because of padding), with which it does nothing, especially given that we already provide a "userData" pointer which is probably also not used frequently. Given that the behavior has been incorrect for years and nobody besides me has complained, I'd be inclined to remove SRID in the next major release.
I would agree, but I think this would constitute a policy change so can't go in 3.6.3.
Could go in 3.7.0 even though we've called beta since it's not really an api change.