Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable relative filenames in ImageElevationDatabase kwl map #290

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

vilhelmen
Copy link

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
Copy link
Member

dburken commented Feb 1, 2023

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
Copy link
Member

dburken commented Feb 1, 2023

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
Copy link
Member

oscarkramer commented Feb 1, 2023

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
Copy link
Author

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
Copy link
Author

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
Copy link
Member

dburken commented Jan 12, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants