Opened 8 years ago

Closed 5 years ago

#1576 closed defect (fixed)

r.in.poly broken by window split

Reported by: hamish Owned by: grass-dev@…
Priority: normal Milestone: 7.0.0
Component: Raster Version: svn-trunk
Keywords: r.in.poly, i.topo.corr, Rast_set_window(), Cc: stefansylla@…
CPU: All Platform: All

Description

Hi,

r.in.poly was missed during the split window transition (r42876) and now exits with an error. (previously there was a suppressed-by-the-module warning)

ERROR: Output window changed while maps are open for write

a simple change in the row paging code of Rast_set_window() to Rast_set_output_window() in r.in.poly/raster.c didn't help; any advice on how the input/output split windows now work? the doxygen lib fn comments briefly explain the what, but not the why of the design or how you're supposed to use it.

thanks, Hamish

Change History (8)

comment:1 Changed 8 years ago by hamish

Cc: stefansylla@… added
Keywords: i.topo.corr added

i.topo.corr in grass7 addons too.

comment:2 in reply to:  1 ; Changed 8 years ago by mmetz

Replying to hamish:

i.topo.corr in grass7 addons too.

Both r.in.poly and i.topo.corr should be fixed now.

Markus M

comment:3 in reply to:  description Changed 8 years ago by glynn

Replying to hamish:

any advice on how the input/output split windows now work?

The input window determines the number of rows and columns for Rast_get_row() etc, i.e. the valid range for the "row" parameter, and the number of elements in the "buf" array.

The output window determines the number of rows and columns for Rast_put_row() etc, i.e. the number of times it should be called ("put" operations don't have a "row" parameter) and the number of elements in the "buf" array.

For most modules, the input and output windows should be the same, but e.g. r.resamp.* will typically use split windows.

The rationale was that the old mechanism was inefficient. It would change the window twice for each row of output (set "read" window, read rows, set "write" window, write row). Each change caused the column mapping to be recalculated for each input map. With split windows, there are separate windows for input maps and output maps, eliminating the need to change back and forth between input and output windows. The column mapping is calculated when a map is opened and never changes. Attempting to change the input window while any input maps are open generates an error, as does attempting to change the output window while any output maps are open.

Also, certain functions assume the existence of a single window, e.g. Rast_get_window(), Rast_window_{rows,cols}(). These functions still exist, and work so long as the windows aren't split (i.e. neither Rast_set_input_window() nor Rast_set_output_window() have been called, or the windows have since been joined by calling Rast_set_window()), but will generate an error if the windows are split. If the windows are split, the code must use e.g. Rast_get_input_window(), Rast_input_window_rows() etc to read the appropriate window.

The "split window" design eliminates the most common reason for changing the window while maps are open, although there may be cases it doesn't cover (e.g. reading multiple input maps at different resolutions won't work). If we need to support that, there are a number of possible solutions.

One is to store the current window in the fileinfo structure whenever the column mapping is created, check that it matches the current window whenever Rast_read_row() etc is called, and either re-generate it or generate an error if it differs. This avoids having to needlessly re-calculate the column mapping for all input maps on every set-window operation, but requires a window comparison (which probably needs some tolerance for comparing floating-point fields) for each get-row operation.

Another is to allow each map to have a separate window, set from the current window when the map is opened. The main drawback is that Rast_window_rows() etc become meaningless. We would need to add yet another "level" of window splitting, so you'd have: single window, read/write windows, per-map windows.

As usual, the main problem isn't implementation, but design and ensuring that existing code doesn't silently break (the current implementation should ensure that any breakage results in a fatal error).

comment:4 Changed 8 years ago by neteler

Please add a synthesis in lib/raster/rasterlib.dox

comment:5 in reply to:  4 Changed 6 years ago by neteler

Replying to neteler:

Please add a synthesis in lib/raster/rasterlib.dox

Done in r59331 (verification in SVN recommended).

comment:6 in reply to:  2 Changed 6 years ago by hamish

Replying to mmetz:

Both r.in.poly and i.topo.corr should be fixed now.

Hi,

fyi I still get the "ERROR: Output window changed while ..." error with r.in.poly with the latest trunk. I used the example from the man page piped through stdin.

thanks, Hamish

comment:7 Changed 6 years ago by hamish

g.region e=591450 w=591300 n=492660 s=492600 -p

r.in.poly in=- out=test1234 << EOF
A
   591316.80   4926455.50
   591410.25   4926482.40
   591434.60   4926393.60
   591341.20   4926368.70
= 42 stadium
EOF

comment:8 Changed 5 years ago by wenzeslaus

Resolution: fixed
Status: newclosed

I don't get any error with the code posted and G7:r.in.poly changed quite a bit. Closing the ticket as fixed. Reopen if you are still getting ...Output window changed while... and please submit a test which shows it.

Note: See TracTickets for help on using tickets.