[GRASS5] [bug #2061] (grass) r.los needs FP update

GRASS GRASS developer
1

this bug's URL: http://intevation.de/rt/webrt?serial_num=2061
-------------------------------------------------------------------------

Subject: r.los needs FP update

grass binary for platform: Compiled from Sources

After inspection of the source code (due to unsatisfying results)
it seems that 'r.los' only works with CELL elevation data:

src/raster/r.los/cmd/main.c:
         /* allocate buffer space for row-io to layer */
         cell = G_allocate_cell_buf();
         /* open elevation overlay file for reading */
         old = G_open_cell_old (elev_layer, old_mapset);
etc.

The module should be fixed to also accept FCELL/DCELL DEMs.
However, I have no clear idea how much work is needed for
that as segmentation is used internally.

Markus

-------------------------------------------- Managed by Request Tracker

2

On Wed, 13 Aug 2003, Request Tracker wrote:

this bug's URL: http://intevation.de/rt/webrt?serial_num=2061
-------------------------------------------------------------------------

Subject: r.los needs FP update

It is my idea for a sort of long-term plan / project that I would like to
re-write r.los, based on an algorithm that was published in the July issue
this year of 'Photogrammetric Engineering & Remote Sensing':
http://www.asprs.org/asprs/publications/pe&rs/2003journal/july/abstracts.html
The title is 'A Fast Algorithm for Approximate Viewshed Computation'
The improvements in r.cva (cumulative viewshed analysis
http://www.ucl.ac.uk/~tcrnmar/GIS/r.cva.html ) would also be incorporated
into this new version. (r.cva also does not work with floating point maps
although it is well written and up-to-date and is a lot better than the
current r.los; the web page doesn't seem to have any information on the
licence though so it is unclear if it can go into the current GRASS)
Anyway the functionality can just be cloned anyway; it would probably be
easier to do this than adapt seeing it is based on the old r.los.

The algorithm in PE&RS is from the US Army CERL so it would be especially
appropriate for it to go into the latest GRASS. I would hope some of the
Vectlib and dglib functions could be used for determining the crossing
points and distances between the cells but I will have to learn about
them first.

Paul

3

On Wed, Aug 13, 2003 at 02:31:02PM +0100, Paul Kelly wrote:

The improvements in r.cva (cumulative viewshed analysis
http://www.ucl.ac.uk/~tcrnmar/GIS/r.cva.html ) would also be incorporated
into this new version. (r.cva also does not work with floating point maps
although it is well written and up-to-date and is a lot better than the
current r.los; the web page doesn't seem to have any information on the
licence though so it is unclear if it can go into the current GRASS)

I've send a polite email to its author to ask for the license.
Without license, noone can use r.cva. :wink:

4

Request Tracker wrote:

Subject: r.los needs FP update

grass binary for platform: Compiled from Sources

After inspection of the source code (due to unsatisfying results)
it seems that 'r.los' only works with CELL elevation data:

src/raster/r.los/cmd/main.c:
         /* allocate buffer space for row-io to layer */
         cell = G_allocate_cell_buf();
         /* open elevation overlay file for reading */
         old = G_open_cell_old (elev_layer, old_mapset);
etc.

The module should be fixed to also accept FCELL/DCELL DEMs.
However, I have no clear idea how much work is needed for
that as segmentation is used internally.

It should probably be changed to just use either FCELL or DCELL
internally. I don't see any reason to preserve the underlying data
type.

AFAICT, the only situations where CELL maps shouldn't be converted to
FP is where they may actually represent discrete categories rather
than a scalar quantity.

--
Glynn Clements <glynn.clements@virgin.net>

5

On Wed, Aug 13, 2003 at 04:27:52PM +0100, Glynn Clements wrote:

Request Tracker wrote:

> Subject: r.los needs FP update
>
> grass binary for platform: Compiled from Sources
>
> After inspection of the source code (due to unsatisfying results)
> it seems that 'r.los' only works with CELL elevation data:
>
> src/raster/r.los/cmd/main.c:
> /* allocate buffer space for row-io to layer */
> cell = G_allocate_cell_buf();
> /* open elevation overlay file for reading */
> old = G_open_cell_old (elev_layer, old_mapset);
> etc.
>
> The module should be fixed to also accept FCELL/DCELL DEMs.
> However, I have no clear idea how much work is needed for
> that as segmentation is used internally.

It should probably be changed to just use either FCELL or DCELL
internally. I don't see any reason to preserve the underlying data
type.

Sounds good - but the segment library (src/libes/segment/) seems
to be limited to CELL.

AFAICT, the only situations where CELL maps shouldn't be converted to
FP is where they may actually represent discrete categories rather
than a scalar quantity.

Help is welcome to get r.los fixed.

Markus

6

Markus Neteler wrote:

> > Subject: r.los needs FP update
> >
> > grass binary for platform: Compiled from Sources
> >
> > After inspection of the source code (due to unsatisfying results)
> > it seems that 'r.los' only works with CELL elevation data:
> >
> > src/raster/r.los/cmd/main.c:
> > /* allocate buffer space for row-io to layer */
> > cell = G_allocate_cell_buf();
> > /* open elevation overlay file for reading */
> > old = G_open_cell_old (elev_layer, old_mapset);
> > etc.
> >
> > The module should be fixed to also accept FCELL/DCELL DEMs.
> > However, I have no clear idea how much work is needed for
> > that as segmentation is used internally.
>
> It should probably be changed to just use either FCELL or DCELL
> internally. I don't see any reason to preserve the underlying data
> type.

Sounds good - but the segment library (src/libes/segment/) seems
to be limited to CELL.

That contradicts the documentation, which implies that the cells may
have arbitrary size; i.e. a cell is just "len" bytes, where "len" is
given in the call to segment_format(). Cursory examination of the code
supports the documentation.

--
Glynn Clements <glynn.clements@virgin.net>

7

On Thu, Aug 14, 2003 at 04:17:14PM +0100, Glynn Clements wrote:

Markus Neteler wrote:

> > > Subject: r.los needs FP update
> > >
> > > grass binary for platform: Compiled from Sources
> > >
> > > After inspection of the source code (due to unsatisfying results)
> > > it seems that 'r.los' only works with CELL elevation data:
> > >
> > > src/raster/r.los/cmd/main.c:
> > > /* allocate buffer space for row-io to layer */
> > > cell = G_allocate_cell_buf();
> > > /* open elevation overlay file for reading */
> > > old = G_open_cell_old (elev_layer, old_mapset);
> > > etc.
> > >
> > > The module should be fixed to also accept FCELL/DCELL DEMs.
> > > However, I have no clear idea how much work is needed for
> > > that as segmentation is used internally.
> >
> > It should probably be changed to just use either FCELL or DCELL
> > internally. I don't see any reason to preserve the underlying data
> > type.
>
> Sounds good - but the segment library (src/libes/segment/) seems
> to be limited to CELL.

That contradicts the documentation, which implies that the cells may
have arbitrary size; i.e. a cell is just "len" bytes, where "len" is
given in the call to segment_format(). Cursory examination of the code
supports the documentation.

OK - I tried to update the code and ended up with
WARNING: can't put float row into integer map

That's why I assumed (after a *quick* grep for CELL in segment)
that's related to segment. So there must be another oversight
in my updated r.los code. Mhhh.

Markus

8

Markus Neteler wrote:

> > Sounds good - but the segment library (src/libes/segment/) seems
> > to be limited to CELL.
>
> That contradicts the documentation, which implies that the cells may
> have arbitrary size; i.e. a cell is just "len" bytes, where "len" is
> given in the call to segment_format(). Cursory examination of the code
> supports the documentation.

OK - I tried to update the code and ended up with
WARNING: can't put float row into integer map

That's a libgis issue; you have to create the output map as FP (with
G_open_fp_cell_new(), G_open_raster_new(..., FCELL_TYPE) etc) if you
want to put FP rows.

If you want to write a CELL map, just open the input map as CELL and
use CELL internally. G_get_raster_row() etc will coerce FCELL and
DCELL to CELL (via quantisation rules), but G_put_raster_row() etc
refuse to coerce FCELL or DCELL to CELL (but will do the other
combinations).

It wouldn't be difficult to make libgis do those coercions, so
presumably they were omitted deliberately. Probably due to the
possibility of overflow.

that's why I assumed (after a *quick* grep for CELL in segment)
that's related to segment.

No; the segment library doesn't know anything about GRASS data types
(and doesn't use any GRASS functions except G_malloc and G_warning).
The only occurrences of CELL in the segment code are in comments:

get_row.c:11:/* int segment_get_row (SEGMENT *SEG, CELL *buf,int row) */
put_row.c:12:/* buf is CELL * WRAT code */
put_row.c:13:/* int segment_put_row (SEGMENT *SEG, CELL *buf,int row) */

The "buf" arguments are actually "void *".

--
Glynn Clements <glynn.clements@virgin.net>

9

On Fri, Aug 15, 2003 at 12:31:10AM +0100, Glynn Clements wrote:

Markus Neteler wrote:

> > > Sounds good - but the segment library (src/libes/segment/) seems
> > > to be limited to CELL.
> >
> > That contradicts the documentation, which implies that the cells may
> > have arbitrary size; i.e. a cell is just "len" bytes, where "len" is
> > given in the call to segment_format(). Cursory examination of the code
> > supports the documentation.
>
> OK - I tried to update the code and ended up with
> WARNING: can't put float row into integer map

That's a libgis issue; you have to create the output map as FP (with
G_open_fp_cell_new(), G_open_raster_new(..., FCELL_TYPE) etc) if you
want to put FP rows.

Right, however I was already using
new = G_open_raster_new (out_layer,FCELL_TYPE);

If you want to write a CELL map, just open the input map as CELL and
use CELL internally. G_get_raster_row() etc will coerce FCELL and
DCELL to CELL (via quantisation rules), but G_put_raster_row() etc
refuse to coerce FCELL or DCELL to CELL (but will do the other
combinations).

I would like to write FCELL as the resulting vertical angle (in degrees)
may be nice not to see as CELL.

It wouldn't be difficult to make libgis do those coercions, so
presumably they were omitted deliberately. Probably due to the
possibility of overflow.

> that's why I assumed (after a *quick* grep for CELL in segment)
> that's related to segment.

No; the segment library doesn't know anything about GRASS data types
(and doesn't use any GRASS functions except G_malloc and G_warning).
The only occurrences of CELL in the segment code are in comments:

get_row.c:11:/* int segment_get_row (SEGMENT *SEG, CELL *buf,int row) */
put_row.c:12:/* buf is CELL * WRAT code */
put_row.c:13:/* int segment_put_row (SEGMENT *SEG, CELL *buf,int row) */

The "buf" arguments are actually "void *".

This was a stupid 'grep' done by me, sorry.

OK, found the problem (forgot to update also the cell value variables 'old'
and 'new').

Later I'll commit the change to CVS.

Thanks

Markus