<div dir="ltr">Even, firstly thanks for your remarks!<br><br><div><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
Not much to say on the RFC itself. Perhaps you should mention who will take<br>
care of committing in the source : Dmitry I guess ?<br></blockquote><div><br></div><div>Yes, I think someone who has committing rights can do this. I will discuss it with Dmitriy. <br></div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
I looked at the code changes:<br>
<a href="https://github.com/MikhanGusev/gdal/compare/OSGeo:trunk...trunk" target="_blank">https://github.com/MikhanGusev/gdal/compare/OSGeo:trunk...trunk</a><br>
<br>
My remarks:<br>
- any reason not to licence  autotest/gnm/gnm_gdalnetwork_test.py<br>
 under X/MIT ? There are a few files LGPL licenced in autotest, but this is<br>
more historical remain than intended policy.<br></blockquote><div> <br></div><div>No problems with X/MIT.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
-  gdal/apps/GNUmakefile : in clean target, analysis/*.o is probably unneeded<br></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
- which gnm headers are installed ? gnm.h gnm_frmts.h gnm_api.h<br>
gnmstdanalysis.h ?<br></blockquote><div> </div>Actually exactly these 4 files must be installed. I'll add them to makefiles.<br><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
- header files should have guards against multiple inclusion. At least seen for<br>
gnm/analysis/gnmstdanalysis.h<br></blockquote><div><br></div><div>My mistake, I'll fix this.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
- in implementations there are many maps, vectors, etc., so I guess some<br>
algorithms may consume a lot of memory if operating on huge networks. The<br>
documentation should make it clear.<br></blockquote><div><br></div><div>The use of these containers make the calculations fast. And what I haven't done actually - is the testing on the very huge graphs. I'll include this to my future plans. No problems with docs. <br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
- GNMGdalStdRoutingAnalyser and GNMGdalStdCommutationsAnalyser: currently have<br>
only one method and no member variables. Do you foresee future extensions to<br>
add member variables ? If not, they might go to GNMGdalStdAnalyser, no ? And<br>
what about non-GDAL network ? Shouldn't the interface of<br>
DijkstraShortestPath() and ResourceDestribution() go to GNMStdAnalyser ? Open<br>
questions : just wondering of what it might look like if other network<br>
implementations are done. I can imagine that the dijkstra alg would have<br>
similar interface.<br></blockquote><div> <br></div><div>According to my idea GNMStdAnalyser is actually the class which firstly represents a graph and the methods to operate it. So it must contain only the basic algorithms. DijkstraShortestPath() and ResourceDestribution() are the methods which use the basic methods of the graph and return the results in the specific form. Also in general for the classes GNMGdalStdRoutingAnalyser and GNMGdalStdCommutationsAnalyser: they were designed as the "specific-purpose" classes approximately for engineering and road networks, so they can be extended in future and probably with member variables. But this relates to the answer for the next remark:<br><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
- gnm/frmts/gnm_frmts.h :  I'm a bit concerned about exposing (installed<br>
header + CPL_DLL) an interface that has not yet been implemented. My intuition<br>
is that it might change once the first one or two implementations have been<br>
made. So maybe keep it internal/experimental for now.<br></blockquote><div><br></div><div>I agree that the inclusion of the interfaces/capabilities that can be (not will be) extended in future is not a 100% good idea. I hoped that someone will be interested or even I will have time to implement and extend something of what I wrote at the "Future ideas" section in my RFC. But we do not know exactly will it be implemented or not. So: (1) We can remove for now all these interfaces "for future", which means to leave only GNMGdalNetwork and one analysing class. (2) Try to implement these capabilities: pgRouting for gnm_frmts.h and for example GNMGdalStdRoutingAnalyser with some algorithm extension (K shortest path). <br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
- gnm/frmts/gnm_frmts.h : "static const char *GNMAllFormats[] = {" should not<br>
be exposed in a .h file<br></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
- gnm/frmts/gnm_frmts.h :  static std::vector<std::string> _regNetFrmts : same<br>
remark as above<br></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
- gnm/frmts/gnm_frmts.h :  GetFormatByName() and RegisterAllNetworkFormats()<br>
should not be in a .h and lack "namespace"<br></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
- gnm/gnm.h : static const char *GNMGdalSupportedDrivers[] should be moved<br>
somewhere else<br></blockquote><div><br></div><div>Actually I have no idea where to place the gnm_frmts.h static fields. They must be common for all formats. But if we decide to remove formats support it will be easier. <br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
- gnm/gnm.h : most #define are not of public use. might be moved in a<br>
gnm_priv.h<br></blockquote><div> <br></div><div>Yes, the same for rule types.<br></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
- gnm/gnm.h : typedef int GNMDirection and the 3 following defines. A typedef<br>
enum would seem more appropriate. Same for GNMErr (although I can see this<br>
pattern in ogr_core.h. not clear why enum have not been chosen)<br></blockquote><div><br></div><div>Agree. <br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
- gnm/gnm.h : GNMManager::Close() the GNMNetwork should know if it is native<br>
or not. The user shouldn't have to specify it again.<br></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
- gnm/gnm.h : GNMManager::GdalCloseNetwork(). Ideally this method should not<br>
exist and Close() should work for all kind of network objects.<br></blockquote><div><br></div><div>Can be possible. <br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
- gnm/gnm.h : GNMNetwork class definition. Commented methods that could be<br>
removed. TODO that no longer apply<br></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
- gnm/gnm.h : IsDriverSupported() --> IsDatasetDriverSupported() for<br>
consistency between name and argument ?<br></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
- gnm/gnm.h : std::pair<char**,int> GetClassLayers () : very unusual interface<br>
in GDAL. I'd say return a char** NULL terminated for consistency to other<br>
methods in the same class<br></blockquote><div> <br></div><div>Agree.<br></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
- gnm/gnm_api.h: all functions are prefixed by GNMGdal. But some methods<br>
actually belong to the base GNMNetwork class, ant not specifically to<br>
GNMGdalNetwork. For example CreateLayer(), CreateFeature(), and many others<br></blockquote><div><br></div><div>Yes, I should update the C header and also wrap some methods from GNMManager.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
- gnm/gnm_tut.dox : #include "analysis/gnmstdanalysis.h" . Is the analysis<br>
directory valid for an installed GDAL ?<br></blockquote><div><br></div><div>Haven't check. <br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
- gnm/gnmgdalnetwork.cpp : _makeNewLayerName(): newName might overflow. instead<br>
of 100, use strlen(name) + 50. Or return a std::string to avoid manual dealing<br>
with memory<br></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
- gnm/gnmgdalnetwork.cpp : GNMGdalNetwork::CreateLayer() : the call to<br>
_makeNewLayerName return a char* not a const char*.<br></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
- GNMGdalNetwork::IsDriverSupported (): CSLFindString() >= 0 could be used to<br>
replace the loop<br></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
- GNMGdalNetwork::GetSupportedDrivers () : return<br>
CSLDuplicate(GNMGdalSupportedDrivers)<br></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
- GNMGdalNetwork::GetMetaParamValueName () : return a "const char*" that must<br>
be freed by delete[]. Quite unusual. Better return a "char*". Other issue, it<br>
is allocated with new char[], but the method is bound to C. C callers cannot<br>
call delete[]. So you have to allocate with CPLMalloc(), and ask the caller to<br>
free with CPLFree(). Isn't there a risk of overflow with 254 characters ?<br></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
- GNMGdalNetwork::GetMetaParamValueDescr () : same remarks as above<br></blockquote><div> <br>I'll fix. <br><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
- GNMManager::CreateConnectivity () : I'm confused by the 'native' term. In<br>
this method, native=false seems to imply the GDAL "proprietary" network format<br>
that can work with any vector driver that has similar capabilities than<br>
shapefile. Whereas in the RFC text, it seems to imply the contrary ( "network<br>
of ”GDAL-native” format").<br></blockquote><div> <br></div><div>Maybe I used it not correctly everywhere, but the general idea is the following: The term "native" corresponds to the existed network formats (so when we work with pgRouting network we work with its native tables and fields, rather than with GNMGdal- system layers), while the GDAL-networks are not "native" and more likely "common".<br></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
-  GNMNetwork::AutoConnect() :" if (lineGFID == 684) { int x = 9; } " debug<br>
left-over ?<span class=""><font color="#888888"><br>
</font></span></blockquote></div><br></div><div class="gmail_extra">Yes, my mistake :D<br></div></div></div>