[gdal-dev] RFC 48: Geographical networks support

[Even Rouault]

Hi Dmitry,

> 
> I have just finished the integration of GSoC2014 work into the
> geographical network support in GDAL.
> The major changes from original code:
> 1) The base network class (GNMNetwork) is inherited form GDALDataset. So
> any GDAL utility, that works with GDALDataset can work with GNMNetwork.
> 2) Geographical network is a new type of data, additionally to the
> DCAP_RASTER and DCAP_VECTOR, added DCAP_GNM
> 3) I created the generic implementation of GDAL geographical network
> (GNMGenericNetwork). The generic network have some features: support of
> connection rules, support virtual vertices and edges, etc. The
> GNMNetwork API can be changed in future as common functions to generic
> network and some other networks (pgRouting, ArcGIS, OSRM, etc.) should
> be moved to the GNMNetwork class.
> 4) Two network drivers created for generic network: file based
> datasource and db based datasource. The file network driver tested on
> ESRI shapefile and db - on PostGIS. The another vector datasource which
> are not applicable with this drivers should have their own drivers.

I could probably figure out that myself by looking at the code, but what are 
the main difference between the generic file network driver and the db file 
network driver ? Both should use the generic OGR API, no ?

> 5) The main method of created and configured GNMNetwork is GetPath,
> which return the temporary OGRLayer (similar to ExecuteSQL).
> 6) By default the GNM is switched off. The ./configure --with-gnm should
> be run to enable GNM. Mayne set it on by default?

I think the decision depends mostly on how much we are confident that the API 
is stable. While there's not a non-generic driver implemented, there's 
potentially a risk (I think, but I've not a strong opinion on this.)

> 7) The python binding is implemented and used in tests.
> 
> The RFC 48 in most cases is actual, except C API, which is not
> implemented.  Do we need it?

The idea is that the SWIG bindings only needs to know the C API, which provide 
some insulation from C++ ABI changes. So you could theoretically use the GDAL 
2.0.0 Python bindings with a GDAL 2.1.0 shared object.
But the main reason might be to avoid issues related to C++ ABI if not using 
the same compiler for the GDAL dll and building the Python bindings. Which can 
happen on Windows since the Python bindings require the same MSVC version than 
the one used to compile Python itself. When sticking to the C ABI, you can be 
safe. With the C++ ABI, some bad things could potentially happen.

> https://trac.osgeo.org/gdal/wiki/rfc48_geographical_networks_support
> 
> Links:

Perhaps do you have a git branch somewhere (just for the sake of easier 
applying) ?

> - diff with binary test data -
> https://drive.google.com/file/d/0BzlLlHyrgQHkNkktUmxYbWtjV3M/view?usp=shari
> ng - diff without binary test data -
> https://drive.google.com/file/d/0BzlLlHyrgQHkZ2lZN2wwTTVOQlE/view?usp=shari
> ng - binary test data -
> https://drive.google.com/file/d/0BzlLlHyrgQHkTTJjRFZNOEtfb3c/view?usp=shari
> ng
> 
> Code reviewing and testing are welcome. Also the some decisions in
> implementation can be discussed.

From a random skimming, I'm wondering about the below :

+%inline %{
+  GNMNetworkShadow* CastToNetwork(GDALMajorObjectShadow* base) {
+    return static_cast<GNMNetworkShadow*>(base);
+  }
+%}
+
+%inline %{
+  GNMGenericNetworkShadow* CastToGenericNetwork(GDALMajorObjectShadow* base) 
{
+    return static_cast<GNMGenericNetworkShadow*>(base);
+  }
+%}
+

Shouldn't that be a dynamic_cast ? Otherwise you'll manage to cast any 
GDALMajorObject into a GNMxxxx even when it is not valid. You could probably 
test that by trying to cast a TIFF dataset or something completely irrelevant.

> 
> The GDAL 2.0 provide ability to create datasets combine vector and
> raster data (drivers support both types).
> In case of PostGIS we can have the driver combined vector, raster and
> networks (pgRouting). Maybe someone give an idea how it can be done.


Currently, from what I see from the generic network implementation, the whole 
graph is loaded from storage into memory at Open() time 
(GNMFileNetwork::Open() calling LoadGraphLayer()). Right ?  Wondering how much 
a limitation it can be in practice. Couldn't the loading of the graph be 
defered up to the point it is really needed, so that Open() is kept reasonably 
fast ? I don't see any try catch around insertions into STL containers, so I 
guess that things might blow up if a too big graph is loaded.

Even

-- 
Spatialys - Geospatial professional services
http://www.spatialys.com