Opened 8 years ago

Closed 8 years ago

#1050 closed enhancement (fixed)

Add GEOS spatial operations to liblwgeom

Reported by: bnordgren Owned by: strk
Priority: medium Milestone: PostGIS 2.0.0
Component: postgis Version: trunk
Keywords: Cc:

Description

Enable "spatial" things with an LWGEOM object from C. Attached patch adds some functions like:

LWGEOM *lwgeom_intersection(LWGEOM *geom1, LWGEOM *geom2) ;
LWGEOM *lwgeom_difference(LWGEOM *geom1, LWGEOM *geom2) ;
LWGEOM *lwgeom_symdifference(LWGEOM* geom1, LWGEOM* geom2) ;
LWGEOM* lwgeom_union(LWGEOM *geom1, LWGEOM *geom2) ;

This is more or less a copy/paste from the intersection/difference/etc. functions which operate on PG_FUNCTION_ARGS. However, I had to move some things from ./postgis to ./liblwgeom :

  • GEOS2LWGEOM
  • LWGEOM2GEOS
  • ptarray_from_GEOSCoordSeq
  • ptarray_to_GEOSCoordSeq
  • lwgeom_geos_error
  • lwgeom_geos_errmsg
  • profile.h (copied, not moved, so it's in two places now)

I very unimaginatively stuck with the name "lwgeom_geos.c" for the file, so there's two of this file: one in postgis and one in liblwgeom.

Did basic cleanup on the functions copied, such that it now compiles without warnings (vars set but not used; converting elog(ERROR, ...) to lwerror() )...

This is NOT TESTED. I'm putting this here early to get feedback before going nuts.

Attachments (1)

lwgeom_operations.patch (41.8 KB) - added by bnordgren 8 years ago.
Updated patch against r7683

Download all attachments as: .zip

Change History (18)

comment:1 Changed 8 years ago by bnordgren

Note: patch generated using svn diff from r7444.

[bnordgren@ghosty-mosty postgis-patch]$ svn status
?       GNUmakefile
M       postgis/lwgeom_geos.c
M       postgis/lwgeom_geos.h
?       liblwgeom/y.output
M       liblwgeom/Makefile.in
A       liblwgeom/profile.h
A       liblwgeom/lwgeom_geos.c
A       liblwgeom/lwgeom_geos.h
[bnordgren@ghosty-mosty postgis-patch]$ svn info
Path: .
URL: http://svn.osgeo.org/postgis/trunk
Repository Root: http://svn.osgeo.org/postgis
Repository UUID: b70326c6-7e19-0410-871a-916f4a2858ee
Revision: 7444
Node Kind: directory
Schedule: normal
Last Changed Author: robe
Last Changed Rev: 7444
Last Changed Date: 2011-06-22 06:55:08 +0000 (Wed, 22 Jun 2011)

comment:2 Changed 8 years ago by pramsey

lwgeom_geos.c has changed since this patch was generated. See #1060, #712

comment:3 Changed 8 years ago by bnordgren

New patch is against r7528.

comment:4 Changed 8 years ago by strk

Owner: changed from pramsey to strk
Status: newassigned
Version: 1.5.Xtrunk

comment:5 Changed 8 years ago by strk

bnordgren: are you running the testsuite with your patch applied ? Please attach a revised patch when it passes it.

comment:6 Changed 8 years ago by strk

Oh, for the record: psql:/home/src/postgis/postgis/regress/postgis.sql:65: ERROR: could not load library "/home/src/postgis/postgis/regress/00-regress-install/lib/postgis-2.0.so": /home/src/postgis/postgis/regress/00-regress-install/lib/postgis-2.0.so: undefined symbol: POSTGIS_DEBUGF

comment:7 Changed 8 years ago by mcayland

I don't think this patch is ready to apply in its current form, mainly because at the moment it copies the various functions into liblwgeom. For this to be applied, we need to alter postgis/lwgeom_geos.c to call the underlying liblwgeom functions so that the main algorithm is in one location only, i.e. so we move the functionality into liblwgeom instead.

I appreciate that there is more work required here in regard to refactoring the caching API, but I don't want two copies of the same algorithm within PostGIS.

comment:8 Changed 8 years ago by bnordgren

I'm hesitant to put more work into refactoring anything until #1133 is resolved. This ticket, #1053, and #1058 are really one big thing to me, so they're my biggest concern at this point. If it's going to take a while to resolve #1133, I'll wait until I can clear out all three tickets, in order to avoid re-doing this patch every time the relevant files are touched.

I believe the original patch moved the affected functions to liblwgeom (e.g., removing them from postgis/lwgeom_geos.c). It's possible that I didn't delete those functions when I recreated the patch, because I was in a hurry. I suspect that is also how the POSTGIS_DEBUGF symbol snuck in there. I don't believe we need to leave these functions in postgis/lwgeom_geos.c at all, as they should get linked in.

comment:9 in reply to:  7 Changed 8 years ago by bnordgren

Replying to mcayland:

I don't think this patch is ready to apply in its current form, mainly because at the moment it copies the various functions into liblwgeom. For this to be applied, we need to alter postgis/lwgeom_geos.c to call the underlying liblwgeom functions so that the main algorithm is in one location only, i.e. so we move the functionality into liblwgeom instead.

Ok, I finally got my head back in the game enough that I get what you're talking about. Originally, I left all the PG_LWGEOM based functions in there because it appeared that maintaining parallel data types with parallel functionality was the style. Done.

Changed 8 years ago by bnordgren

Attachment: lwgeom_operations.patch added

Updated patch against r7683

comment:10 Changed 8 years ago by bnordgren

The current patch passes all the cunit tests. My development environment won't currently run any tests which require connecting to a live server (see dev list). A cursory glance suggests that there are no cunit tests for intersection, union, etc. Perhaps any sql based tests should be ported to cunit in the future. They should also remain as sql based tests, however, as there is plenty of opportunity to cut and paste something incorrectly.

New patch is against r7683 and has "moved" instead of "copied" the relevant functions.

comment:11 Changed 8 years ago by strk

I'll be dealing with this in a week or two.

comment:12 Changed 8 years ago by strk

Resolution: fixed
Status: assignedclosed

Committed in r7696 Linking and building flags also updated in subsequent commits, where I've also moved a couple more functions. Sounds about time to move more GEOS functions, do you feel like providing another patch bnordgren, while I look at cross-platform ways to produce a shared library ?

comment:13 Changed 8 years ago by bnordgren

Resolution: fixed
Status: closedreopened

Somehow, the function prototypes for lwgeom_intersection() and friends didn't get added to source:/trunk/liblwgeom/lwgeom_geos.h@7720. I double checked and they're in the patch. Should be very quick to fix.

comment:14 in reply to:  12 Changed 8 years ago by bnordgren

Replying to strk:

Sounds about time to move more GEOS functions, do you feel like providing another patch bnordgren, while I look at cross-platform ways to produce a shared library ?

I'm discovering that maintaining software as a set of separate-but-dependant patches is a lot more work than just having a single tree that you don't bother to keep in sync with the repo. In any case, I'm really only familiar with the very basics of GEOS usage which I had to learn in the course of writing this code, so I may not be a very good choice.

comment:15 Changed 8 years ago by bnordgren

Ack!

As it turns out, I need "intersects" to be migrated. Because I can't afford to get distracted with moving all of the relationship tests, the plan for the moment is to fold the migration of "intersects" into my "generation 2" implementation of #1058 / seamless spatial wiki page. I can work on moving the remainder of the relationship tests for completeness's sake.

comment:16 Changed 8 years ago by strk

The prototypes were moved to liblwgeom.h, which will be the public header. All the others are meant to be "internal". At least that seemed to be the best short-term plan to me.

comment:17 Changed 8 years ago by strk

Resolution: fixed
Status: reopenedclosed

I don't think there's anything more to do here. Moving more things could be the subject of a new ticket

Note: See TracTickets for help on using tickets.