ST_Clip(raster, geom) include new method to pixel selection: touch

[latot]

Hi all, actually ST_Clip does not always return all the pixels that intersect with the geometries, I think would be great to have a param to be able to select all the pixes that the geometry touches.

I was thinking initially, a param to "if_touches = true", but then... what happens if in the future there is other implementations of how to select pixels? for that, maybe is good to have a param like "method" to choose the one we want to use.

No idea which would be the best case, because I don't know if there is a simpler way to send custom params to all the "methods" that can be developed.

But maybe this last part is a separate issue, I just think is good to have consideration on it.

Thx!

[robe]

I've always been bothered by this myself. Always have to do some hackish thing like buffer my geometry to get back all.

[robe]

Looking at this closer looks like ST_Clip eventually uses

GDALRasterizeGeometries function to burn the geometry into an empty raster. The options is not set

Looking at the GDALRasterizeGeometries function in GDAL repo, it has a clause

 * @param papszOptions special options controlling rasterization
 * "ALL_TOUCHED": May be set to TRUE to set all pixels touched
 * by the line or polygons, not just those whose center is within the polygon
 * or that are selected by brezenhams line algorithm.  Defaults to FALSE

So I think right now the options postgis raster is passing is NULL so I'm guessing it's defaulting to not touched and that's why we have the annoying behavior of rasterized geometry not taking up the whole pixel being left out.

My plan is to add a new boolean argument to ST_Clip -- touched, and the default, I guess for backward compatibility we should set to "False", but I'd really love the default to be set to true.

But anyway I think this needs to go into 3.5.0 (not prior), to allow exposing this new "touched" arg.

The other question is ​https://postgis.net/docs/manual-dev/en/RT_ST_Clip.html, do we add to all protos. I had no idea we had so many, as I've only ever used it as ST_Clip(rast,geom) which corresponds to this signature

raster ST_Clip(raster rast, geometry geom, double precision[] nodataval=NULL, boolean crop=TRUE);

so that I'd definitely replace with below (though that would be a breaking change), so maybe touched=FALSE would be safer. But I suspect most people prefer true, or assume touched=true already

raster ST_Clip(raster rast, geometry geom, double precision[] nodataval=NULL, boolean crop=TRUE, boolean touched=TRUE);

[latot]

Hi! Get a touched param would be great! also the same thought about the default value.

About the docs about GDALRasterizeGeometries I think needs to be improved.

The brezenhams algorithms works only for pure lines, so is more likely pass over the center of the pixel would works for polygons, which leads me to two points, one would be what happens with points? and confirm if this is trully the behavior.

Ideally get the behavior per type of geometry, if in geometry collection the function is applied on all internal geoms we would only need to know for basic ones, point, line, polygon.

[robe]

I have work in progress here - ​https://git.osgeo.org/gitea/postgis/postgis/pulls/166

I decided to stick with default touched=false for backward compatibility and that is also the default for ST_AsRaster (which already has a touched argument) - ​https://postgis.net/docs/manual-dev/en/RT_ST_AsRaster.html

I still need to add tests for the touched case and fix the upgrade. strk -- I don't think your replaces likes "double precision", so assuming I have to use float8?

[robe]

Okay still having issue with the upgrade

psql:/woodpecker/src/git.osgeo.org/gitea/postgis/postgis/regress/hooks/hook-after-upgrade.sql:29: NOTICE:  Dropping function st_clip_deprecated_by_postgis_305(raster,integer,geometry,double precision,boolean)
psql:/woodpecker/src/git.osgeo.org/gitea/postgis/postgis/regress/hooks/hook-after-upgrade.sql:29: ERROR:  cannot drop function st_clip_deprecated_by_postgis_305(raster,integer,geometry,double precision,boolean) because other objects depend on it
DETAIL:  view upgrade_test_raster_view_st_clip depends on function st_clip_deprecated_by_postgis_305(raster,integer,geometry,double precision,boolean)
HINT:  Use DROP ... CASCADE to drop the dependent objects too.
CONTEXT:  SQL statement "DROP FUNCTION st_clip_deprecated_by_postgis_305(raster,integer,geometry,double precision,boolean)"
PL/pgSQL function inline_code_block line 10 at EXECUTE
-----------------------------------------------------------------------------
make: Leaving directory '/woodpecker/src/git.osgeo.org/gitea/postgis/postgis/build/pg12/raster/test/regress'
FAIL: postgis_raster extension upgrade 3.0.9--3.5.0dev!
Cleaning up

psql:/woodpecker/src/git.osgeo.org/gitea/postgis/postgis/regress/hooks/hook-after-upgrade.sql:29: NOTICE:  Dropping function st_clip_deprecated_by_postgis_305(raster,integer,geometry,double precision,boolean)
psql:/woodpecker/src/git.osgeo.org/gitea/postgis/postgis/regress/hooks/hook-after-upgrade.sql:29: ERROR:  cannot drop function st_clip_deprecated_by_postgis_305(raster,integer,geometry,double precision,boolean) because other objects depend on it
DETAIL:  view upgrade_test_raster_view_st_clip depends on function st_clip_deprecated_by_postgis_305(raster,integer,geometry,double precision,boolean)
HINT:  Use DROP ... CASCADE to drop the dependent objects too.
CONTEXT:  SQL statement "DROP FUNCTION st_clip_deprecated_by_postgis_305(raster,integer,geometry,double precision,boolean)"
PL/pgSQL function inline_code_block line 10 at EXECUTE
-----------------------------------------------------------------------------
make: Leaving directory '/woodpecker/src/git.osgeo.org/gitea/postgis/postgis/build/pg15/raster/test/regress'
FAIL: postgis_raster extension upgrade 3.2.5--3.5.0dev!
Cleaning up

[strk]

That view should be removed by the raster after-upgrade hook as it is created by the raster before-upgrade hook ( ​https://woodie.osgeo.org/repos/30/pipeline/1600/20#L8752 )

but in the CI log we see that the core after-upgrade ( ​https://woodie.osgeo.org/repos/30/pipeline/1600/20#L8755 ) is invoked before the raster after-upgrade, which does not get a chance of being run.

This is weird, because the RUNTESTFLAGS_INTERNAL seem to correctly pass the after-upgrade hook of raster: ​https://woodie.osgeo.org/repos/30/pipeline/1600/20#L8740

I'll try to reproduce locally, were you able to ?

[strk]

Note you can reduce the cost of reproducing the test by doing something like this:

MAKE_ARGS=TESTS=regress/core/regress utils/check_all_upgrades.sh 3.5.0dev!

[strk]

"easy" command line to reproduce the problem:

MAKE_ARGS=TESTS=regress/core/regress \
 utils/check_all_upgrades.sh \
   -s \
   --skip '(postgis extension|topology|unpackaged|3\.[0123])' \
   3.5.0dev!

[strk]

I'm afraid this is a problem with the script itself, using wrong order of switches, with my RUNTESTFLAGS_INTERNAL trick not being good enough and needing an improvement (before hooks need to be put after core ones, after hooks need to be put before core ones).

Closer-to-problem call to reproduce it:

make -C raster/test/regress check \
  RUNTESTFLAGS="-v --extension --upgrade-path 3.4.0dev--3.5.0dev!" \
  TESTS=raster/test/regress/box3d

[strk]

I'm thinking we can possibly avoid the pre-upgrade hook specific for raster as long as we use the global common pre and post hooks which are now capable of creating views for each and every available function

[robe]

Replying to strk:

I'm thinking we can possibly avoid the pre-upgrade hook specific for raster as long as we use the global common pre and post hooks which are now capable of creating views for each and every available function

So you are saying get rid of the ST_clip view in raster hook-before-upgrade-raster.sql or should I get rid of that file altogether

[robe]

I left the file, but removed the st_clip views in the before and after. My upgrade tests pass locally now. Testing on woodpecker now.

[robe]

I removed the ST_Clip view tests from hook-before-upgrade-raster.sql and the after that drops it. i think I got further but now woodie is erroring with

ERROR:  function "_st_clip" already exists with same argument types
-----------------------------------------------------------------------------
make: *** [/woodpecker/src/git.osgeo.org/gitea/postgis/postgis/regress/runtest.mk:24: check-regress] Error 3
make: Leaving directory '/woodpecker/src/git.osgeo.org/gitea/postgis/postgis/build/pg15/raster/test/regress'
FAIL: postgis_raster script hard upgrade unpackaged3.2--:auto
Cleaning up

I checked and I did add a clause at top of _st_clip

-- Replaces _st_clip(rast raster, nband integer[], geom geometry, nodataval float8[], crop boolean) deprecated in 3.5.0

so not sure what I could be missing now

[strk]

IIRC the Replaces syntax does not allow argument names, but I should check. Maybe see the other Replaces. Yes we really need that documentation for developers, I'm still not sure where to put it though

[robe]

Replying to strk:

IIRC the Replaces syntax does not allow argument names, but I should check. Maybe see the other Replaces. Yes we really need that documentation for developers, I'm still not sure where to put it though

It should because some of the past ST_Clip were just argument name changes.

Also I am seeing this - I seeing in the autogenerate postgis_restore.pl in the skip section this:

But hmm maybe it doesn't. I do see you postgis.sql.in don't have names. Okay I'll change

FUNCTION _st_clip(raster, integer[], geometry, double precision[], boolean, boolean)

Which is correct, so not sure why it's not skipping it and why this would even be in a postgis 3.2 backup to stumble across

[robe]

I also had changed all not have the argument names, but that did not fix the issue.

I noticed the _st_clip old signature wasn't in the generated postgis_restore.pl skip list so I added to the static postgis_restore.pl.in and that got passed the _st_clip

Now I am getting

SET
SET
SET
SET
ERROR:  function "st_clip" already exists with same argument types
-----------------------------------------------------------------------------
make: *** [/woodpecker/src/git.osgeo.org/gitea/postgis/postgis/regress/runtest.mk:24: check-regress] Error 3
make: Leaving directory '/woodpecker/src/git.osgeo.org/gitea/postgis/postgis/build/pg12/raster/test/regress'
FAIL: postgis_raster script hard upgrade unpackaged2.5--:auto
Cleaning up

Do I have to add every single one I removed in this list, I thought it is supposed to pick this up.

[strk]

the problem should be fixed as of [d59e89e20d13a0045ddd37a4775de79f6cb9c423/git]

[Regina Obe <lr@…>]

In 4fa9e10/git:

ST_Clip with option of touched

Supporting touched similar to ST_AsRaster
Closes #5496
Closes ​https://git.osgeo.org/gitea/postgis/postgis/pulls/166

[Sandro Santilli <strk@…>]

In e94b8f2/git:

Re-add raster upgrade test view dropped in 4fa9e106e

References #5488
References #5496

[Sandro Santilli <strk@…>]

In c5730e6c/git:

Add credit to INIA-CSIC for funding ST_Clip for touch pixel selection

References #5496