ossimlabs / ossim

Core OSSIM (Open Source Software Image Map) package including C++ code for OSSIM library, command-line applications, tests, and build system
MIT License
295 stars 142 forks source link

Enable relative filenames in ImageElevationDatabase kwl map #290

Open vilhelmen opened 1 year ago

vilhelmen commented 1 year ago

The file map is a great addition, but it's not as flexible as it could be. I'm interested in hearing opinions on enabling relative paths in the file map. This allows for preexisting maps to be modified to work in environments where centralized elevation data may be mounted in different locations. Relative paths are a nice feature of VRTs, but I've been experiencing projection issues when doing elevation queries (#289 but I haven't had the opportunity to document the issue on the dev branch, and it's most likely an issue with the gdal plugin, not ossim core).

In order to sidestep potential issues with Windows, a file entry is only appended to connectionString if it starts with a period (instead of a non-slash).

The current patch works and does not change default behavior, but the addition of connectionString to ossimImageElevationDatabase::ossimImageElevationFileEntry::loadState may not be preferred. Doing it in loadMapFromKwl feels better, but the way initialization is offloaded to loadState prevents that.

dburken commented 1 year ago

Instead of doing that lets change the code to work with relative or absolute file names in the map. Basically:

if ( file.isRelative() ) file = ossimFilename(m_connectionString).dirCat( file );

We would need a flag to save the map file entry relative or absolute.

Then if relative you just change the connection string if you move it in the ossim preferences file.

dburken commented 1 year ago

So basically this: elev_cell0.file: /data1/elevation/general_raster_1arc/N38W119.hgt elev_cell0.grect: (39,-119,0,WGE),(38,-118,0,WGE) elev_cell1.file: /data1/elevation/general_raster_1arc/N40W118.hgt elev_cell1.grect: (41,-118,0,WGE),(40,-117,0,WGE) elev_cell2.file: /data1/elevation/general_raster_1arc/N39W118.hgt elev_cell2.grect: (40,-118,0,WGE),(39,-117,0,WGE) elev_cell3.file: /data1/elevation/general_raster_1arc/N40W119.hgt elev_cell3.grect: (41,-119,0,WGE),(40,-118,0,WGE) elev_cell4.file: /data1/elevation/general_raster_1arc/N39W120.hgt elev_cell4.grect: (40,-120,0,WGE),(39,-119,0,WGE) elev_cell5.file: /data1/elevation/general_raster_1arc/N38W120.hgt elev_cell5.grect: (39,-120,0,WGE),(38,-119,0,WGE) elev_cell6.file: /data1/elevation/general_raster_1arc/N38W118.hgt elev_cell6.grect: (39,-118,0,WGE),(38,-117,0,WGE) elev_cell7.file: /data1/elevation/general_raster_1arc/N39W119.hgt elev_cell7.grect: (40,-119,0,WGE),(39,-118,0,WGE) elev_cell8.file: /data1/elevation/general_raster_1arc/N40W120.hgt elev_cell8.grect: (41,-120,0,WGE),(40,-119,0,WGE)

Would become: elev_cell0.file: N38W119.hgt elev_cell0.grect: (39,-119,0,WGE),(38,-118,0,WGE) elev_cell1.file: N40W118.hgt elev_cell1.grect: (41,-118,0,WGE),(40,-117,0,WGE) elev_cell2.file: N39W118.hgt elev_cell2.grect: (40,-118,0,WGE),(39,-117,0,WGE) elev_cell3.file: N40W119.hgt elev_cell3.grect: (41,-119,0,WGE),(40,-118,0,WGE) elev_cell4.file: N39W120.hgt elev_cell4.grect: (40,-120,0,WGE),(39,-119,0,WGE) elev_cell5.file: N38W120.hgt elev_cell5.grect: (39,-120,0,WGE),(38,-119,0,WGE) elev_cell6.file: N38W118.hgt elev_cell6.grect: (39,-118,0,WGE),(38,-117,0,WGE) elev_cell7.file: N39W119.hgt elev_cell7.grect: (40,-119,0,WGE),(39,-118,0,WGE) elev_cell8.file: N40W120.hgt elev_cell8.grect: (41,-120,0,WGE),(40,-119,0,WGE)

oscarkramer commented 1 year ago

The loadState() and saveState() virtuals with two args (kwl, prefix) are declared WAY up the inheritance tree to ossimObject. Adding a special loadState() just for that class is not a good idea, given how the whole object factory system is set up. I agree with Burken on his potential solution.

vilhelmen commented 1 year ago

I agree that the change to loadState is ugly and a bad idea, but it's not clear to me how else we can correct the filename during FileEntry load/save.

We could put a flag in the preferences file like this with a default to absolute to expose it to the user:

elevation_manager.elevation_source1.filename: /ELEVATION/aster3
elevation_manager.elevation_source1.type: image_directory
elevation_manager.elevation_source1.map: relative

But FileEntry can't see the preferences file or the connection string during load/save, so it can't tell when to save a relative path or where to cut the path to make it relative, loadState can't build an absolute path when given a relative one, and we don't want to interfere with inheritance by modifying their signature to accept more data.

We could create a new FileEntry constructor that takes a reference to m_connectionString and a boolean flag to save relative paths and use it during loadMapFromKwl to fix loading and slip it into processFIle to fix saving. That's the cleanest way I can think of, but changing load/save behavior based on constructor values feels iffy. Would a new constructor be an inheritance issue?

We could (if possible) rewrite the kwl pre- and post-save, but that feels like a string nightmare.

I'm more than happy to do the legwork to make this happen, but I don't see a nice approach to doing it.

vilhelmen commented 1 year ago

Slept on it and I'm wondering what benefits absolute paths provide over relative paths? An absolute map duplicates the preference file connection string in a way that's somewhat hidden from the user and is very brittle because it can't survive the image directory being moved. A relative path has a small startup cost, but it's really just some string concatenation.

I don't think the elevation kwl map has been in a release yet, maybe it would be best to switch to relative paths only? We just need to find a way to inject the connection string into FileEntry so it can save and load right.

dburken commented 7 months ago

Just another comment on this. You can simply make the connection string a directory now provided you are OK mapping all the images in there. Like this: elevation_manager.elevation_source6.connection_string: $(OSSIM_DATA)/elevation/srtm/1arc elevation_manager.elevation_source6.enabled: 1 elevation_manager.elevation_source6.type: image_directory elevation_manager.elevation_source6.mean_spacing: 30 elevation_manager.elevation_source6.min_open_cells: 25 elevation_manager.elevation_source6.max_open_cells: 50 elevation_manager.elevation_source6.memory_map_cells: 0 elevation_manager.elevation_source6.geoid.type: geoid1996

Test: ossim-info --height 27.9 -81 -T ossimImageElevationDatabase ossimImageElevationDatabase::loadState result=true elevation.info.cell: /data1/elevation/srtm/1arc/N27W081.hgt elevation.info.geoid_name: geoid1996 elevation.info.geoid_offset: -29.3161998748779 elevation.info.gpt: (27.9,-81,0,WGE) elevation.info.height_above_ellipsoid: -4.31619987493614 elevation.info.height_above_msl: 24.9999999999418