Add SFCGAL support

[colivier]

From a dev point of view:

• PostGIS side, the (SF)CGAL stuff is very thin, cf patch attached. (As clever part of the 3d stuff, are located in CGAL and SFCGAL.)

• We bring some cunit test (quite the same than GEOS ones) and pass them (valgrind clean also)

• For all CGAL functions who should behave the same in 2D than GEOS ones We pass all PostGIS regress tests, except few of them, where tests themself are not so well designed cf: <​https://github.com/Oslandia/postgis-sfcgal/blob/master/bench/postgis_regress_delta.txt>

We plan to propose further solutions to enhance this.

For a user point of view:

• All SFCGAL functions are located in sfcgal schema

• SFCGAL functions provided, some are the same than GEOS ones, and obviously some are brand news:

sfcgal.ST_GeomFromEWKT(text) sfcgal.ST_Intersects(geometry, geometry) sfcgal.ST_3DIntersects(geometry, geometry) sfcgal.ST_Intersection(geometry, geometry) sfcgal.ST_3DIntersection(geometry, geometry) sfcgal.ST_DelaunayTriangles(geometry) sfcgal.ST_Area(geometry) sfcgal.ST_Distance(geometry, geometry) sfcgal.ST_3DDistance(geometry, geometry) sfcgal.ST_MakeSolid(geometry) sfcgal.ST_Tesselate(geometry) sfcgal.ST_3DArea(geometry) sfcgal.ST_HasPlane(geometry) sfcgal.ST_Extrude(geometry, float8, float8, float8) sfcgal.ST_ForceLHR(geometry) sfcgal.ST_Orientation(geometry) sfcgal.ST_Minkowski(geometry, geometry) sfcgal.ST_Offset(geometry, float8, int4) sfcgal.ST_StraightSkeleton(geometry) sfcgal.ST_Round(geometry, int4)

[colivier]

Current diff

[strk]

Do you want to keep the "sfcgal" out of the search_path ? I'm asking because functions with the same name would be confusing in that case.

[colivier]

Hi Sandro,

Paul suggestion here, was:

• Use a PostgreSQL parameter to switch between GEOS/SFCGAL/any future other lib

for common topology functions

• So no need userland to deal with sfcgal prefix

[colivier]

Humm was'nt aware of this timeline, took time on this week end to commit it

[robe]

colivier,

Great. Yah sorry for pushing the issue. But PostgreSQL 9.3beta1 is going to come out next week and if we want wide adoption of 2.1 we are going to have to catch the boat so to speak before it takes off.

[robe]

Olivier,

Sorry I should have looked closely at the patch. I see 2 main issues

1) Your SQL functions can't be folded into postgis.sql, they have to be in a separate file similar to what is done for topology and tiger geocoder. This is for 2 reasons mostly because of the way extensions work

a) functions in an extension can't be different based on whether or not you compile with something.

b) sfcgal function are in a separate schema and all functions in an extension NEED TO BE in same schema.

c) You need to be able to install sfcgal as an extension for it to make the 9.1+ cut. If you need help with this, I can help out. That means you will need a folder in /extensions folder similar to what is done with postgis, postgis_topology, postgis_tiger_geocoder

That said please make these changes before committing (might be better for you to just attach another patch and I'll inspect to see if I see any glaring issues). If you can't we really need to push this to 2.2.

[colivier]

Regina,

First the previous patch is becoming a bit old, several improvement have been made around Boston Code Sprint.

I reply to your points

a) yeap already done to separate postgis,sql and sfcgal.sql (and same with uninstall)

b) we did too, with a PostgreSQL env var called postgis.backend to be able to switch from one backend to an another,

without adding a new schema.

c) Humm this one we did'nt do !

I look on it at once i finalize the regress step Thanks for the help offer !

For your information the current patch attached I'm still quite confident to finalize it soon enough for 2.1

[robe]

Olivier,

Looks better, but I see some issues with attached patch

1) ST_3DIntersects change is going to cause problems for people who want to use this but have just a 2D index (not a 3d one). I think 3d index can use && but 2D index can't use &&&

2) Why are the functions in postgis.sql.in being touched: ST_Area(geometry) , ST_Distance

3) postgis/lwgeom_geos.h note sure the reason for these additions as well.

strk, Paul, Mark you better step in. I think this patch is over my head to fully judge.

[colivier]

Regina,

1) Humm your right i assume index on 3D data must be 3D Index but no way to be sure, i remove the &&& to a safe &&

2) and 3)

As we want to have a single SQL Schema (and that 's the best option for end user) we need to have a single backend, able to perform the switch (i.e geos or sfcgal).

So for some functions provided by both GEOS and SFCGAL, we need to slightly change the prototype name in postgis.sql.in and lwgeom_geos.h.

But obviously we didn't change anything in the GEOS behaviour functions

[robe]

2,3 - that's a problem since it means users can't add sfcgal support after installing postgis. Something admittedly we have an issue with with GEOS versions, but that I'D REALLY like to get away from especially since it has the uncomfortable effect of an extension requiring another extension to be compiled in a specific way.

I much preferred the old way where sfcgal was in its own schema and users just add it to their search_path. I wouldn't be so worried if this was more tested before we fold it in beta. Have you tested this on 9.3 to see if it even works? If it doesn't it CAN'T go into 2.1.

On another note — why is run_test being changed as well. That seems to have nothing to do with sfcgal as far as I can tell.

[colivier]

2 & 3) SFCGAL behave as an optional library to be activated on PostGIS compile time (like json lib for instance)

So yes if you did not choosed it on install time, you have to reinstall PostGIS binary.

The previous schema stuff obliged the end user to know wich library provide each function This point was discussed on Boston too, and if you want it back you have to convince Paul

And OK i will check with 9.3 too

(run_test: it's only a local change in my copy for locale issues

and this obviously will not be commited)

Thanks for you detailled code review !

[robe]

2 & 3) hmm okay. It's not ideal but livable. since they are packaged together and since you have the scgal functions compiled into postgis-2.1 lib I guess it will be obvious to folks when the extension can't install so not as bad as bad as our GEOS version issues.

Also make sure to test without configuring with sfcgal to make sure all tests past.

[robe]

Olivier,

I already see problems. I applied your patch to a clean download and fail on configure with this:

Using user-specified geos-config file: /c/ming64/projects/geos/rel-3.4.0devw64/b
in/geos-config
checking GEOS version... 3.4.0dev
checking geos_c.h usability... yes
checking geos_c.h presence... yes
checking for geos_c.h... yes
checking for initGEOS in -lgeos_c... yes
checking for sfcgal-config... no
configure: error: sfcgal-config cannot be found.

The default behavior should be I should be able to compile without sfcgal.

My configure looks like this:

./configure \
 --build=x86_64-w${OS_BUILD}-mingw32  \
  --with-xml2config=${PROJECTS}/libxml/rel-libxml2-2.7.8w${OS_BUILD}/bin/xml2-config \
  --with-pgconfig=${PROJECTS}/pgx${OS_BUILD}/pg${PG_VER}/bin/pg_config \
  --with-geosconfig=${PROJECTS}/geos/rel-${GEOSVER}/bin/geos-config \
  --with-projdir=${PROJECTS}/proj/rel-4.8.0w${OS_BUILD} \
  --with-gdalconfig=${PROJECTS}/gdal/rel-${GDAL_VER}/bin/gdal-config \
  --with-jsondir=${PROJECTS}/json-c/rel-${JSON_VER}w${OS_BUILD} \
  --with-libiconv=${PROJECTS}/rel-libiconv-1.13.1w${OS_BUILD} \
  --with-xsldir=${PROJECTS}/docbook/docbook-xsl-1.76.1 \
  --with-gui --with-gettext=no

where all the variables are just so I can compile with different variants of stuff.

[colivier]

Yeap Regina,

This patch was a current extract, not the one i intend to commit

Still working on something cleaner…

Thanks for the alpha tests !

[colivier]

Regina,

After have a look on configure stuff, the behaviour is indeed the same than raster (if you don't have gdal-config, you have to specify —without-raster to pass configure step)

I added a more explicit error message, so now it will be: configure: error: sfcgal-config cannot be found. Use —without-sfcgal or install sfcgal

[robe]

It would be better if it was more like json where if you don't specify json-dir it doesn't build with it if it can't find it or its not specified. Probably best to discuss this on the list because I can see going both ways and not saying I'm right. The other issue is what if people want to build with different versions of sfcgal in future (liek they have both 2.1 and 2.2 and don't want them pointing at same scfcgal) etc. How are they going to do that? So you will need a flag to specify the directory at some point anyway. Might as well have that NOW.

The reasons I would prefer it not be done that way at least for 2.1 is:

1) It's gonna break the buildbots so requires me patching them before I can even commit this patch and I have to make the configure different for 2.0 than it is for 2.1 2) raster we decided would be part of core postgis extension so it was a crippling change to encourage it. and we discussed it long and hard and it may have been the wrong decision to make it part of core extension.

3) Raster was tested for a long time before we reached beta so people were prepared for the configure change

We can discuss switching the configure to without-sfcgal in 2.2, but for 2.1, I really don't want people to have to change what they do in configure steps unless they want the new features and the features are more thoroughly tested by a larger crowd.

Besides I can see once pramsey introduces his point cloud thing, we may have a lot of

—without-this-and-that-and-that

So just to get a basic install people are going to have to do nothing but write WITHOUT everywhere. topology admittedly was a special case, because I really thought it was stupid we allow 2.0 to be compiled with anything less than GEOS 3.3 given how much stuff is disabled .

[robe]

slight correction —- I see you do have a

--with-sfcgal=PATH

So you can keep the without-sfcgal for the case where people have cgal libraries but don't want them used. Which could be the case for package maintainers building various flavors.

However the default behavior should be

if you can't find sfcgal and its not specified in —with-sfcgal, then just don't build with it.

[colivier]

Regina,

OK i've got your point, right now the configure behaviour is so becoming:

• If sfcgal-config not founded → sfcgal disabled
• if sfcgfal-config founded and —without-sfcgal → sfcgal disabled
• if sfcgal-config not founded and —with-sfcgal → configure stop on error message

(And we still have the —with-sfcgal=PATH to choose one SFCGAL version among severals)

[robe]

Perfect

[robe]

I'm moving this to new component section sfcgal. I'll add the document ticket to that as well.

[colivier]

First commit as r11389

[colivier]

For the record:

Functions available now both with GEOS and SFCGAL

• ST_Intersects
• ST_3DIntersects
• ST_Intersection
• ST_Area
• ST_Distance
• ST_3DDistance

2D functions should behave quite the same, 3D functions should be improved (handling more geometry type)

New functions available only when SFCGAL support activated

• postgis_sfcgal_version()
• ST_3DIntersection(geom1 geometry, geom2 geometry)
• ST_Tesselate(geometry)
• ST_3DArea(geometry)
• ST_Extrude(geometry, float8, float8, float8)
• ST_ForceLHR(geometry)
• ST_Orientation(geometry)
• ST_Minkowski(geometry, geometry)
• ST_StraightSkeleton(geometry)

New function related to SFCGAL stuff, but in PostGIS land:

• ST_force_sfs(geometry)

[robe]

Okay I'll test this out and we should raise for vote.

[robe]

colivier,

I see you already committed the patch. Don't do that again for such a big patch that touches so many file. Bborie fixed the build break for you out of the kindness of his heart at r11391

Really for something this huge psc and dev group should have had time to officially approve on the list and do a spot check before it went in.

[colivier]

Regina,

I did'nt received build build break email ! (is that normal ?)

And thanks to Bborie for r11391 !

The code itself is indeed really close than previous patch provided here, at the beginning of the week i merely added regress tests, documentation and so on so for me it was OK to commit it, without further notice

[robe]

I might not have your email in place in new bots. actually not absolutely sure I have email enabled. I may just have it posting to irc. ​http://irclogs.geoapt.com/postgis/%23postgis.2013-05-09.log