Make default raster image path relative to tileindex file

[sdlime]

--- From mapserver-dev ---

The attached path makes the path of image files for a TILEINDEX layer relative to the directory of the TILEINDEX shapefile, if the map's SHAPEPATH is not set.

I prefer having the path fall back to the TILEINDEX file's directory rather than the mapfile directory, because the tile index file and the raster imagery tend to be colocated, on a large volume separate from the mapfile. Maybe there are some good reasons for relative-to-mapfile behavior too, but I couldn't think of any. Behavior is unchanged if the SHAPEPATH is set.

The patch also removes an unused variable from msDrawRaster() and prints out the full (instead of relative) path of a raster file if that file could not be opened. Printing the full path helped me with error resolution; some might consider it less secure to be displaying it though.

-Dave Fuhry

[sdlime]

patch

[sdlime]

Vector tiling should behave the same as rasters so we'd need to patch that too.

Steve

[dfuhry]

I can update the patch to do a similar thing in msTiledSHPOpenFile().

A potentially cleaner option might be to, in msDrawLayer() before the calls to msDrawRasterLayer() and msDrawVectorLayer():

cwd = getcwd() /* store current working directory */
if (!map->shapepath)
  if (layer->tileindex)
    chdir() to tileindex's directory

then after the msDrawRasterLayer() / msDrawVectorLayer() call:

chdir(cwd) /* restore original working directory */

That would simplify the path building logic and avoid a lot of calls to msBuildPath(). If anything in between does relative file I/O it'll be a problem though. Suggestions as to which approach to take?

[sdlime]

I do believe there is some relative IO (for example, font files or database-based symbology that references an image file), so the more local approach in your patch is probably the way to go.

cc'ing Dan for another opinion.

Steve

[sdlime]

Frick, had the wrong username for Dan. Dave, will your patch apply against the dev tree as is sits now.

Steve

[dfuhry]

Steve,

Yes, the patch still applies (just offset 2 lines).

I haven't updated it any further; I know we want to get the vector (shapefile) tile code synced up with this behavior. I'm willing to do that, but won't have time to update it until this weekend.

[dfuhry]

I added a new patch which performs the same tileindex-relative path logic to shapefile tiles.

The code is currently untested and probably has bugs. On the plus side, it does get rid of some cut-and-paste code blocks, and cuts the worst-case number of calls to msShapefileOpen in half.

The new patch needs testing by myself and others.

[sdlime]

Dave: Have you done any further testing? I could apply and we get folks to beat on it as part of beta3...

Steve

[sdlime]

I've applied but haven't committed. The logic looks okay and the only change I made was changing strcpy to strncpy just to be safe. However, I was wondering if there are any backwards compatibility issues where old tile indexes wouldn't work as a result. Admittedly it's been years since I wrote that code and I'm not sure what could break.

Steve

[sdlime]

The heck with it, I committed in r7744. I can't see how this will break things since nobody will have tiled data stored relative to a mapfile. It's always full path (unaffected) or relative to the SHAPEPATH. I'll make sure to ask folks to specifically test tiled data...

Moving to the documentation component for now...

Steve

[assefa]

committed fixed for windows build r7745.

[sdlime]

There was an issue with the 1st patch in that it assumed that the layer had a tile index defined. I added back the original code in the untiled case so that location relative to the mapfile is taken into account.

Steve

[dfuhry]

I've been using the raster tileindex changes in production for a few months. That patch should be OK with Steve's fix for non-tileindexed raster layers.

I haven't tested the shapefile tileindex changes yet; I don't have a good example dataset handy. I may have a chance this evening.

BTW Assefa: From your commit diff, I gather that MSVC requires all variables to be declared at the top of the function body? I'll try to submit patches that conform.

[assefa]

Replying to dfuhry:

BTW Assefa: From your commit diff, I gather that MSVC requires all variables to be declared at the top of the function body? I'll try to submit patches that conform.

That is correct.

[pramsey]

Re-opened due to user issue:

"Beta3 seems to have broken my tiled raster layers. These layers all worked fine up through 5.2.0-beta2. Is there something I need to change in my config?

The error I get is "msDrawMap(): Image handling error. Failed to draw layer named 'L10'. msDrawRaster(): Unable to access file. L10/512000_0-1024000_512000.jpg using full path L10/512000_0-1024000_512000.jpg"

In map I have SHAPEPATH set to "PWTS/2006_00" In the layers I have TILEINDEX "L10/TILEINDEX" In the tileindex I have Locations such as "L10/512000_0-1024000_512000.jpg"

The file system layout is:

./AERIAL_2006/ramsey_2006.map ./AERIAL_2006/PWTS/2006_00/L10/TILEINDEX.{shp,shx,dbf} ./AERIAL_2006/PWTS/2006_00/L10/*.jpg

Jim Klassen"

[dfuhry]

The newly attached patch should fix it; it works for my testcases with a relative SHAPEPATH, absolute SHAPEPATH, and no SHAPEPATH.

[pramsey]

What about when the data is relative to the mapfile path? My concern is that we not break any existing installation, and the old code tested both shapepath relative and mappath relative.

[pramsey]

mapraster.c patch applied to trunk at r7760

[dfuhry]

I don't think that's true for raster. I'm looking at mapraster.c r7418 and line 1414 is just:

msBuildPath3(szPath, map->mappath, map->shapepath, filename);

If you're referring to the mapshape.c patch, then I think you are correct. (I'm beginning to wish I had put the two patches in separate threads.) I wish there was a way to know which of the two paths is correct before looping through the tileindex files. It will never be the case that half of the tileindex children are in the mappath and the other half are in the shapepath.

Testing all three of {mapfile_path, shapepath, tileindex_path} for every tile file is not the way to go. I don't see any good way to allow new behavior for mapshape.c tileindexes, if we are unwilling to allow any breakage.

I think the halving of fopen() calls for half of the users, and the elimination of error reporting & administrative ambiguity ("a/b/file1: File not found. c/d/file1: File not found". Which is the problem?) is worthy enough to justify a change at some point.

Thoughts? I'm unsure of how to best proceed here.

[pramsey]

It's a bit of a fopen-fest anyways, so I'm not losing sleep over them. Besides, with proper large file support, people are going to stop tile-indexing shapes anyways, right? :)

[pramsey]

Fix to previous patch to restore old behaviors and add the new tile-index-relative behavior.

[pramsey]

I've applied the patch to make tiled shapes behave the way they used it, but to use the relative-to-tile behavior in the case of failure in the relative-to-shapepath case.