Opened 20 years ago

Closed 20 years ago

#671 closed defect (fixed)

msAdjustExtent() is Quirky

Reported by: warmerdam Owned by: sdlime
Priority: high Milestone:
Component: MapServer C Library Version: 4.3
Severity: normal Keywords:
Cc:

Description

Basic problem is that msAdjustExtent() does not produce an extent with the
exact aspect ratio of the map size due to odd use of integer adjustments. 

On entering msAdjustExtent():

(gdb) p *rect
$15 = {minx = 0, miny = 0, maxx = 400, maxy = 300}
(gdb) p width
$16 = 500
(gdb) p height
$17 = 500
(gdb) 

A cellsize of 0.8 is selected.  But this code selects an offset in y that
is an integer number of cellsizes, why?

  ox = MS_NINT(MS_MAX((width - (rect->maxx - rect->minx)/cellsize)/2,0)); //
these were width-1 and height-1
  oy = MS_NINT(MS_MAX((height - (rect->maxy - rect->miny)/cellsize)/2,0));

After applying in the following code we get the reported extents. 

  rect->minx = rect->minx - ox*cellsize;
  rect->miny = rect->miny - oy*cellsize;
  rect->maxx = rect->maxx + ox*cellsize;
  rect->maxy = rect->maxy + oy*cellsize;

(gdb) p *rect
$19 = {minx = 0, miny = -50.400000000000006, maxx = 400, 
  maxy = 350.39999999999998}

Note that the extents rectangle is now 400 wide, but 400.8 high, not
the 1:1 aspect ratio of the passed in size.   I believe the problem is 
the use of MS_NINT in computing the ox and oy values.  I would like to 
just remove that.

I am running into subtle problems in my rotation code with matching zero
angle rotation results against the results derived from the old method
though in theory it is causing minor inaccurancies for other things too.

Change History (7)

comment:1 by fwarmerdam, 20 years ago

To be clear, my suggestion solution is to change this:

  ox = MS_NINT(MS_MAX((width - (rect->maxx - rect->minx)/cellsize)/2,0)); //
these were width-1 and height-1
  oy = MS_NINT(MS_MAX((height - (rect->maxy - rect->miny)/cellsize)/2,0));

To this:

  ox = MS_MAX((width - (rect->maxx - rect->minx)/cellsize)/2,0); //
these were width-1 and height-1
  oy = MS_MAX((height - (rect->maxy - rect->miny)/cellsize)/2,0);

I just hesitate to apply the change because I don't know why the MS_NINT()
was there in the first place.   I would add I don't understand the comment. 


comment:2 by sdlime, 20 years ago

The comment is there because of an error I found when we converted from a 
center-based pixel model to a upper-left corner-based pixel model. If I recall 
the problem was identified by Doug Nebert when using some global lat/lon 
extents and image sizes that should have resulted in no adjustments being made 
to the extent.

It's been so long since that code was written I honestly don't recall why the 
call to MS_NINT is there. I would recommend making your suggested change and 
see if that stabilizes your other computations. (i.e. go for it)

Steve

comment:3 by sdlime, 20 years ago

I just looked into a javascript version of AdjustExtent that I have and it 
looks like:

function AdjustExtent(extent, width, height) 
{
  var cellsize = Math.max((extent[2] - extent[0])/(width-1), (extent[3] - extent
[1])/(height-1));

  if(cellsize > 0) {
    var ox = Math.max(((width-1) - (extent[2] - extent[0])/cellsize)/2,0);
    var oy = Math.max(((height-1) - (extent[3] - extent[1])/cellsize)/2,0);

    extent[0] = extent[0] - ox*cellsize;
    extent[1] = extent[1] - oy*cellsize;
    extent[2] = extent[2] + ox*cellsize;
    extent[3] = extent[3] + oy*cellsize;
  }	

  return(cellsize);
}

No rounding, but the height/width are offset by one. I need to change that.

Can you confirm my logic. No offset makes sense if the extent goes from the UL 
corner of the UL map pixel to the LR corner of the LR pixel. The old pixel 
model-based extent was from the center of the UL map pixel to the center of the 
LR pixel, hence the offset by 1. No offset of image dimensions should be 
correct.

Steve

comment:4 by fwarmerdam, 20 years ago

Steve,

I think your Javascript would be correct if you use width instead of width-1
in the various places. 

I have made the change to msAdjustExtent() in maputil.c, but I'm not
planning to commit it till I commit the rest of the rotation stuff.  Likely
Monday.

comment:5 by sdlime, 20 years ago

Frank: Did you commit your changes? Just wondering if we can close this one out...

Steve

comment:6 by fwarmerdam, 20 years ago

Change applied and somewhat tested.  The msautotest suite has been updated
to account for the extent changes and the rotation work. 

comment:7 by fwarmerdam, 20 years ago

Resolution: fixed
Status: newclosed
Note: See TracTickets for help on using tickets.