Opened 20 years ago

Closed 16 years ago

Last modified 14 years ago

#673 closed defect (fixed)

layer->project flag not being reset properly

Reported by: warmerdam Owned by: warmerdam
Priority: high Milestone: 5.0.1 release
Component: MapServer C Library Version: 4.3
Severity: normal Keywords:
Cc: sgillies@…, mapserver@…, sdlime

Description (last modified by sdlime)

Currently the layer->project flag is MS_TRUE if a layer needs (or potentially
needs) reprojection and if MS_FALSE we know we don't have to look at the
projection object at all. 

In various places, such as mapdraw.c:msDrawShape() we have code constructs
like this:

#ifdef USE_PROJ
    if(layer->project && msProjectionsDiffer(&(layer->projection),
&(map->projection)))
      msProjectPoint(&layer->projection, &map->projection, &center);
    else
      layer->project = MS_FALSE;
#endif

If it finds that the map and layer projections are identical then it
sets the "project" flag to false so we don't have to do any more checking.

However, in a MapScript or other long lived session operating on a map it is
possible to thereafter reset the map projection (such that it would differ
from a layer projection) but not have the layer->project flag reset to 
MS_TRUE.  This can basically result in layers not being reprojected
when they should under some esoteric situations. 

The problem is slightly worse with map rotation in the picture as the
msProjectionsDiffer() also has to check if there is a rotation in effect
and if so, returns TRUE even if the projections don't differ since there is
a transformation to be applied. 

I think the solution is to review all code that uses the layer->project
flag and ensure that it is recomputed carefully on each pass.  Thus
msDrawLayer() might reset the flag to MS_TRUE each time it is called. But
the flag is also used in various query code and possibly other places, so
I think other places also need to be updated. 

I propose to implement a msLayerSetProjectFlag(layerObj*) that would just
set the layer->project flag appropriate at that point depend on whether
projections exist, USE_PROJ is set, and the projections differ.  This could
be inserted at strategic points.  

Technically this issue exists in 4.2 and earlier versions but I think the
situations in which it can cause problems are sufficiently esoteric that 
we can just address it in 4.3.

Change History (18)

comment:1 by dmorissette, 20 years ago

Do you mean changing the construct to:

#ifdef USE_PROJ
    if(msLayerSetProjected(layer))
      msProjectPoint(&layer->projection, &map->projection, &center);
#endif

The above would simplify the code but would not solve the caching effect that
you noticed. Instead of resetting the flag all the time and risking that we miss
one case, should we not instead reset the layer->project flags in the
loadProjection() function and also create a function to set the rotation that
resets that flag for all layers?

comment:2 by sgillies@…, 20 years ago

Cc: sgillies@… added

comment:3 by mapserver@…, 20 years ago

Cc: mapserver@… added

comment:4 by fwarmerdam, 20 years ago

Daniel,

We would need to reset all the layer "project" flags anytime a layer or
map projection changes or if the geotransform being applied to a map 
(ie. rotation, shearing, non-square pixels) changes.  This is a fair number of
places to update.

You are right there is a danger of missing some spots but a global search
on ->project should work pretty well.  

comment:5 by dmorissette, 19 years ago

Milestone: 4.4 release
Frank, would it be enough to reset the layer->project flags inside
msMapComputeGeotransform() and in loadProjectionString()? Wouldn't that cover
all the situations that you describe in comment #2?

comment:6 by sdlime, 17 years ago

Description: modified (diff)

Is this still an issue (can't imagine so)? Would love to close if not.

Steve

comment:7 by warmerdam, 17 years ago

Cc: sdlime added
Owner: changed from sdlime to warmerdam

Steve,

It seems plausible to me that this could still be an issue. Let me assume ownership of this and see if I can reproduce the problem now with mapscript.

comment:8 by warmerdam, 17 years ago

Milestone: 5.0 release5.0.1 release

Sorry, I just never got to this. Pushing to 5.0.1.

comment:9 by warmerdam, 16 years ago

Status: newassigned

The following python mapscript test seems to demonstrate that this problem still exists. the img2.png is blank unless I comment out the img1 draw and save in which case it is fine.

def bug_673():
   
    if string.find(mapscript.msGetVersion(),'SUPPORTS=PROJ') == -1:
        return 'skip'

    map = mapscript.mapObj('../misc/ogr_direct.map')

    map.setProjection('+proj=utm +zone=11 +datum=WGS84')

    layer = map.getLayer(0)

    # Draw map without reprojection.

    layer.setProjection('+proj=utm +zone=11 +datum=WGS84')
    img1 = map.draw()

    # Draw map with reprojection

    map.setProjection('+proj=latlong +datum=WGS84')
    map.setExtent(-117.25,43.02,-117.21,43.05)

    img2 = map.draw()

    img1.save( 'img1.png' )
    img2.save( 'img2.png' )

    return 'success'

Digging for a fix.

comment:10 by warmerdam, 16 years ago

I have applied a fix by just resetting layer->project to MS_TRUE in msDrawLayer(). So each time a layer is drawn this should cause MapServer to reconsider whether or now reprojection is required or not. This still leaves some holes for scripts drawing individual shapes without going through the drawlayer machinery. But this seems like an esoteric case, and I can't really think of a solution for it that doesn't completely disable use of the "project" flag to avoid expensive projection comparisons.

Steve, the fix is r6999 in trunk. Could you confirm that this approach seems sensible before I port it back to 5.0 branch?

comment:11 by warmerdam, 16 years ago

added msautotest/mspython/bug_check.py (test bug_673) to regression test this problem.

comment:12 by sdlime, 16 years ago

I guess I can't think of any other way around this other than to reset it before a draw operation. I wonder if this would be an issue in the query code then as well. I think it could. The msQueryBy... functions are more self contained that the drawing code. It might be enough to add:

lp->project = MS_TRUE;

just inside the layer for loops. There's already a block of code there to reset the result cache just in case the layer is being used in a long running process so that would be the place to stick this too.

Steve

comment:13 by warmerdam, 16 years ago

Steve,

Yes, I see they have the same issue.

I have applied similar changes in these functions as r7016. I haven't really tested them though, other than to build them. So if we see problems with queries in the near future keep this change in mind.

I'll proceed to migrate these two changes into 5.0 branch as r7019.

BTW, I think we could optimize things somewhat by setting layer->project at the top of the msDrawLayer() and query functions and then just checking it later without the repeated test of msProjectionsDiffer() for each shape. If you don't object, I'll make this change but just in trunk.

comment:14 by sdlime, 16 years ago

Your proposed optimization sounds fine to me. Thanks!

Steve

comment:15 by warmerdam, 16 years ago

Resolution: fixed
Status: assignedclosed

Actually, on second thought, I'm not so clear that msDrawLayer() is the only way to get to stuff like msDrawShape() so I'm not so sure the optimization is safe.

I'm just going to close this bug since the bug is fixed. We can consider optimization as a separate issue.

comment:16 by assefa, 14 years ago

Working on a separate bug (#2079), I came across this bug but this time the issue was fixed by setting the layer->project = MS_TRUE in msDrawQueryLayer. In my case, this happened with an SLD with a Filter Encoding using the same projection as the map and a wms bbox request using a projection that differs from the map. The combination did expose the issue.

Any issues If I did commit my changes?

Thanks

--- mapdraw.c (revision 9546) +++ mapdraw.c (working copy) @@ -1121,6 +1121,10 @@

if(!msLayerIsVisible(map, layer)) return(MS_SUCCESS); /* not an error, just n

othing to do */

+ /* conditions may have changed since this layer last drawn, so set + layer->project true to recheck projection needs (Bug #673) */ + layer->project = MS_TRUE; +

comment:17 by dmorissette, 14 years ago

Since msDrawlayer() used to do this already, and what you are proposing is adding the same to msDrawQueryLayer(), then I think it's safe to add.

comment:18 by assefa, 14 years ago

Thanks, committed in r9549.

Note: See TracTickets for help on using tickets.