Opened 6 years ago

Closed 6 years ago

#4005 closed enhancement (fixed)

GDALPolygonize should also be able to read raster values into a 32bit float buffer

Reported by: pracine Owned by: warmerdam
Priority: normal Milestone:
Component: Algorithms Version: svn-trunk
Severity: normal Keywords:
Cc: jorgearevalo

Description

And even a 64bit double if possible.

This is necessary for PostGIS raster to be able to polygonize many elevation or temperature rasters.

Change History (10)

comment:1 Changed 6 years ago by jorgearevalo

Cc: jorgearevalo added

An alternative GDALPolygonize2 function may be a good idea, to avoid side effects.

comment:2 Changed 6 years ago by jorgearevalo

Resolution: fixed
Status: newclosed

Implemented in r22476.

comment:3 Changed 6 years ago by Even Rouault

Resolution: fixed
Status: closedreopened

Jorge,

you might want to revise the doxygen comment for the new function. The below paragraph, taken from GDALPolygonize(), seems to no longer apply.

	 * Note that currently the source pixel band values are read into a
	 * signed 32bit integer buffer (Int32), so floating point or complex
	 * bands will be implicitly truncated before processing. 

It might also be good to add a link in the doxygen comment of GDALPolygonize() to point to GDALFPolygonize(), and vice versa.

I've some doubts about the implementation of the equals() function. "int aInt = *(int*)&A", A being a float seems risky because it will violate strict aliasing. With some compilers and some optimization options, it might generate a no-op and aInt could be filled with garbage, a float* being not a valid alias for a int*. Here's what g++ -O2 gives :

fpolygonize.cpp: In function ‘GBool equals(float, float)’:
fpolygonize.cpp:58: warning: dereferencing type-punned pointer will break strict-aliasing rules
fpolygonize.cpp:63: warning: dereferencing type-punned pointer will break strict-aliasing rules

I've been bitten by this in the past: see http://trac.osgeo.org/gdal/changeset/15097/trunk/gdal/ogr/ogrsf_frmts/xplane

I'd encourage you doing a memcpy(&aInt, &A, 4) instead.

I'm also wondering how portable this function is for a big-endian CPU ... It might need some byte-swapping but I'm not 100% sure

Might also be good to add a static keyword before the declaration of the equals() method, to avoid namespace collisions with other code...

comment:4 Changed 6 years ago by Even Rouault

r22477 /trunk/gdal/alg/makefile.vc: Fix windows compilation (untested) (#4005)

comment:5 Changed 6 years ago by jorgearevalo

Thanks for your comments Even. I'll commit the needed changes ASAP.

comment:6 Changed 6 years ago by jorgearevalo

Quick question about Doxygen comments. If I want to reference another function in a different file, should I use @see and/or @file?

comment:7 Changed 6 years ago by Even Rouault

Apparently, just putting the function name, e.g. "GDALPolygonize()", is sufficient for doxygen to figure out it is a function and create the hyperlink.

comment:8 Changed 6 years ago by jorgearevalo

Some changes in r22500:

  • The "equals" name has been replaced by "GDALEqualsFloat". The function is not static because is used in 2 sources. I think the name change will avoid collisions. Anyway, it probably needs to be reviewed for portability
  • Doxygen comments added in GDALPolygonize and GDALFPolygonize.
  • Memcpy used. I forgot to mention in the commit message. Sorry for that.

comment:9 Changed 6 years ago by Even Rouault

r22501 /trunk/gdal/alg/ (fpolygonize.cpp polygonize.cpp): Compilation fix for previous commit; fix doxygen links to other functions (#4005)

comment:10 Changed 6 years ago by Even Rouault

Resolution: fixed
Status: reopenedclosed

r22502 /trunk/gdal/alg/ (gdal_alg_priv.h gdalrasterfpolygonenumerator.cpp): Linking fix for ./configure --without-ogr (#4005)

Note: See TracTickets for help on using tickets.