Opened 17 years ago

Closed 17 years ago

#2015 closed defect (fixed)

cellsize computations suffer from off-by-one error

Reported by: sdlime Owned by: sdlime
Priority: high Milestone: 5.0 release
Component: MapServer C Library Version: svn-trunk (development)
Severity: normal Keywords:
Cc: nsavard

Description (last modified by dmorissette)

The macro MS_CELLSIZE and function msAdjustExtent use width/height in their
computations but should be using width-1 and height-1. The effect is subtle but
needs to be fixed.

Steve

Attachments (1)

MapServer Extent.pdf (445.6 KB ) - added by sdlime 17 years ago.
Graphic showing the problem

Download all attachments as: .zip

Change History (14)

comment:1 by sdlime, 17 years ago

Cc: warmerdam@… dmorissette@… added
Status: newassigned
I have committed a fix for MS_CELLSIZE and msAdjustExtent to CVS HEAD to see
what affect this will have. Have not touched 4.10...

Steve

comment:2 by sdlime, 17 years ago

Maybe 5.0 is where we end this confusion by switching to a OWS extent
interpretation and an UL pixel model. That way 0,0 in image coords will match
minx,maxy in the extent (as it does now) but MapServer extents will match OWS
extents.

Steve

comment:3 by sdlime, 17 years ago

I would propose fixing 4.10 for issues with MS_CELLSIZE, msAdjustExtent and the 
OWS interfaces to use the current models for pixels and extents...

Steve

comment:4 by sdlime, 17 years ago

Note, I've started a draft RFC (25) to get this cleaned up in 5.0... I'm 
especially interested in Frank's comments regarding the raster layer rendering 
code.

Steve

comment:5 by fwarmerdam, 17 years ago

Steve,

Sorry for the lack of response.  Your findings conflict with the mental 
model I had of how things were working so I feel I need to do some digging
to review.  I agree fixing this and making all assumptions clear for 5.0
is a good idea.  But I am leery about possible implications for existing
map files and mapscript scripts. 

comment:6 by sdlime, 17 years ago

Please dig away. Note that the changes committed this morning to map.h and 
maputil.c are what is necessary to fix the cellsize computation issue I believe 
exists. These would need to be applied to 4.10 to make that right. In addition 
the WCS and perhaps the WMS client code would need tweaking to account for 
differences. The WMS server code appears to be accounting for the OWS/MapServer 
extent definition differences, as does the GDAL drawing code.

I really don't think mapfiles or scripts would be impacted much. Most folks use 
MapServer for graphics and simple queries. A change like this to 4.10 would 
likely go unnoticed, but should be disclosed.

Anyway, let me know what questions you might have.

Steve

by sdlime, 17 years ago

Attachment: MapServer Extent.pdf added

Graphic showing the problem

comment:7 by sdlime, 17 years ago

My proposed course of action would be to:

For 4.10:
  - alter MS_CELLSIZE to divide by (d-1) in map.h
  - fix offet equations in msAdjustExtent to use width-1, height-1
  - make sure OGC code differentiates between a MapServer extent and 
    an OGC extent (e.g. fix bug 1983)

That gets all the math right for standard MapServer and OGC interfaces.

For 5.0:
  - propose using the OGC extent interpretation instead of the current one
  - perhaps consider a change to the pixel model

Steve

comment:8 by dmorissette, 17 years ago

Cc: nsavard added
Description: modified (diff)
Milestone: 5.0 release

Steve, are the changes in r5895 (2007-01-22) in MapServer 4.99 final?

I ask this because Norm noticed that the msautotests on GetFeatureInfo have a difference in the reported query coordinate location which I suspect may be related to this change, and we want to make sure that's correct before updating the tests.

comment:9 by sdlime, 17 years ago

I'm almost positive they are. I fixed that several months ago related to a WCS bug, but I will verify that when I get a chance later today and cofirm here.

Steve

comment:10 by sdlime, 17 years ago

I checked and there are 2 changes in 5.0 (answer to your question above is yes).

1) in msAdjustExtent() in maputil.c, we use image dimensions-1 when computing the new extent

2) in mapserver.h the MS_CELLSIZE macro used d-1 when computing cellsize

Those changes match the center pixel and extent model. The WCS code does the conversion now to convert between OWS bbox and a MapServer bbox (e.g. shrink by .5 a pixel). I believe the WMS server code does the same.

Two things to check.

1) Does the WMS layer code make that allowance? (e.g. expand by .5 pixel) 2) are the calls using MS_CELLSIZE ok?

For 2, I did a quick grep of the C code and can verify that the usage in mapserv.c, maptemplate.c, and maputil.c. In those cases a cellsize is being computed from a MapServer extent. There are calls to MS_CELLSIZE in mapwms.c and mapwmslayer.c that I have not checked.

Steve

comment:11 by sdlime, 17 years ago

Version: 4.10svn-trunk (development)

comment:12 by sdlime, 17 years ago

I've found 2 other places that needed to be changed. Remember that with a pixel center to center extent model we need to use width-1 or height-1 when computing scale or cellsize. I discovered (via #2166) that the scale code needed that change (the comments were there saying that was the original way it was before things got confused in 4.x). See r6529 for the 2 changes I made. Things with regards to point/scale zooming for the CGI behave correctly now.

I still need to spend some time with the WMS code before candidate releases.

Steve

comment:13 by sdlime, 17 years ago

Resolution: fixed
Status: assignedclosed

The WMS problem is contained in another bug so closing this one.

Steve

Note: See TracTickets for help on using tickets.