Opened 12 years ago

Closed 11 years ago

#2369 closed defect (fixed)

Make default raster image path relative to tileindex file

Reported by: sdlime Owned by: pramsey
Priority: normal Milestone: 5.2 release
Component: MapServer C Library Version: unspecified
Severity: normal Keywords:
Cc: dfuhry@…, dmorissette, jmckenna

Description

--- 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

Attachments (4)

mapserver-raster-abs-tileindex.patch (1.3 KB) - added by sdlime 12 years ago.
patch
ms_shape_abs_tileindex.v1.patch (11.5 KB) - added by dfuhry 11 years ago.
ms_mapraster_correct_rel_shapepath.v1.patch (778 bytes) - added by dfuhry 11 years ago.
ms_shape_abs_tileindex_v1_fix.patch (9.2 KB) - added by pramsey 11 years ago.
Fix to previous patch to restore old behaviors and add the new tile-index-relative behavior.

Download all attachments as: .zip

Change History (25)

Changed 12 years ago by sdlime

patch

comment:1 Changed 12 years ago by sdlime

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

Steve

comment:2 Changed 12 years ago by 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?

comment:3 Changed 12 years ago by sdlime

Cc: morissette added
Status: newassigned

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

comment:4 Changed 11 years ago by sdlime

Cc: dmorissette added; morissette removed

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

Steve

comment:5 Changed 11 years ago by 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.

Changed 11 years ago by dfuhry

comment:6 Changed 11 years ago by 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.

comment:7 Changed 11 years ago by sdlime

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

Steve

comment:8 Changed 11 years ago by 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

comment:9 Changed 11 years ago by sdlime

Component: MapServer C LibraryMapServer Documentation

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

comment:10 Changed 11 years ago by assefa

committed fixed for windows build r7745.

comment:11 Changed 11 years ago by 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

comment:12 Changed 11 years ago by 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.

comment:13 in reply to:  12 Changed 11 years ago by 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.

comment:14 Changed 11 years ago by jmckenna

Cc: jmckenna added
Component: MapServer DocumentationMapServer C Library

comment:15 Changed 11 years ago by pramsey

Owner: changed from sdlime to pramsey
Status: assignednew

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"

Changed 11 years ago by dfuhry

comment:16 Changed 11 years ago by dfuhry

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

comment:17 Changed 11 years ago by 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.

comment:18 Changed 11 years ago by pramsey

mapraster.c patch applied to trunk at r7760

comment:19 Changed 11 years ago by 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.

comment:20 Changed 11 years ago by 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? :)

Changed 11 years ago by pramsey

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

comment:21 Changed 11 years ago by pramsey

Resolution: fixed
Status: newclosed

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.

Note: See TracTickets for help on using tickets.