Support RDB #1538
Support RDB #1538
Conversation
|
Can you update your pull request to use the new Sphinx based documentation in gdal/docs/source ? |
|
I will do that, as soon as I'm done with the overviews and update the pull request then. |
* removes html frmt_rdb.html * adds rdb.rst * some code cleanup
| @@ -0,0 +1,122 @@ | |||
| #ifndef RDB_DATASET_INCLUDED | |||
There was a problem hiding this comment.
please add a header with copyright & license here as well
gdal/frmts/rdb/rdbdataset.hpp
Outdated
| static int Identify(GDALOpenInfo *poOpenInfo); | ||
|
|
||
| CPLErr GetGeoTransform(double *padfTransform) override; | ||
| const char *_GetProjectionRef() override; |
There was a problem hiding this comment.
_GetProjectionRef() is useless here as GetSpatialRef() has a dedicated implementation not calling the GetSpatialRefFromOldGetProjectionRef() compatibility shim that would call itself _GetProjectionRef()
gdal/frmts/rdb/rdbdataset.cpp
Outdated
|
|
||
| double RDBRasterBand::GetNoDataValue(int *pbSuccess) | ||
| { | ||
| if(pbSuccess == nullptr) |
There was a problem hiding this comment.
pbSuccess == nullptr should not cause the method to return 0 prematurely.
This is just below that you should protect the two *pbSuccess = TRUE / FALSE affectations against a potential nullptr
gdal/frmts/rdb/rdbdataset.cpp
Outdated
| } | ||
| const char *RDBDataset::_GetProjectionRef() | ||
| { | ||
| ReadGeoreferencing(); |
There was a problem hiding this comment.
call to ReadGeoreferening() should be moved to GetSpatialRef()
There was a problem hiding this comment.
I removed that function and did not put it into GetSpatialRef() since that function is const and ReadGeoreferening() is called in the constructor anyway.
|
|
||
| CPLErr RDBDataset::GetGeoTransform(double *padfTransform) | ||
| { | ||
| padfTransform[0] = dfXMin; |
There was a problem hiding this comment.
so this is a "bottom-up" image ?
There was a problem hiding this comment.
yes, since the Webmercator coordinates start in southwest of the earth.
There was a problem hiding this comment.
since the Webmercator coordinates start in southwest of the earth.
not sure that it answers my question, I just wanted to share attention that in most (like 99%) raster formats line 0 of the image is the one that is displayed at the top of your screen, that is the line with the max georeferenced Y value, hence padfTransform[3] is generally maxY, and padfTransform[5] a negative value. But I assume you've verified by gdal_translate'ing to GeoTIFF and displaying in QGIS or whatever that you get a correctly positionned raster.
There was a problem hiding this comment.
I see. You are correct. I used gdal_translate and dumpoverviews in order to create tif files from the .mpx file and checked if the data fits to a map.
|
|
||
| .. shortname:: RDB | ||
|
|
||
| .. versionadded:: 3.x |
gdal/frmts/rdb/rdbdataset.cpp
Outdated
| double GetNoDataValue(int *pbSuccess) override | ||
| { | ||
| double dfInvalidValue = RDBRasterBand::GetNoDataValue(pbSuccess); | ||
| if(pbSuccess == nullptr) |
There was a problem hiding this comment.
see comment of RDBRasterBand::GetNoDataValue() regarding pbSuccess == nullptr
gdal/frmts/rdb/rdbdataset.cpp
Outdated
| } | ||
| try | ||
| { | ||
| RDBDataset *poDS = new RDBDataset(poOpenInfo); |
There was a problem hiding this comment.
if exceptions might occur, it might be wiser to wrap the new in a std::unique_ptr, and .release() it in the return
| if(poWkt != nullptr) | ||
| { | ||
| osWktString = json_object_get_string(poWkt); | ||
| oSpatialReference.importFromWkt(osWktString.c_str()); |
There was a problem hiding this comment.
a call to oSpatialReference.SetAxisMappingStrategy(OAMS_TRADITIONAL_GIS_ORDER); should likely be added in case you return a geographic CRS whose official axis order is lat, long (like EPSG:4326 WGS84), but that the raster horizontal axis is the longitude one (as usual)
|
Not for now. Thank you for reviewing. I plan on building gdal with the rdblib in travis and appveyor, however, this needs easier access to the rdblib in order to this automatically. I will move this in a future pull request if this is okay. Should that be even in the master branch? |
That can. For a few Travis configurations, we pull already build GDAL against proprietary libs when they can be downloaded for free and without authentication. |
|
ok, I will do that, when this is possible It seems that some Travis runs Fail in code that is, I think, not changed by my code. Can you rerun them or are they going to fail again anyway? |
I've restarted them. They unfortunately fail randomly quite often for unknown reason. |
|
This time around all of them passed. For now, I'm not planing to change anything in the context of this pull request. |
What does this PR do?
Adds a driver for reading *.mpx files in the RIEGL RDB format. Needs an external, proprietary library.
What are related issues/pull requests?
Tasklist
Environment