[QGIS-trac] Re: [Quantum GIS] #2987: Auto GCP detection library

[Quantum GIS]

#2987: Auto GCP detection library
--------------------------------+-------------------------------------------
   Reporter:  jamesm6162        |              Owner:  timlinux     
       Type:  patch             |             Status:  assigned     
   Priority:  minor: annoyance  |          Milestone:  Version 1.6.0
  Component:  C++ Plugins       |            Version:  Trunk        
   Keywords:                    |   Platform_version:               
   Platform:  Debian            |           Must_fix:  No           
Status_info:  0                 |  
--------------------------------+-------------------------------------------

Comment(by timlinux):

 Hi James

 I have some initial feedback for you bulleted out below. My comments
 relate more to the code and ui style at the moment and not on the quality
 of algorithm implementation etc.

 First impressions: UI: You have done a great job with the user interface,
 nice and cleanly done (more comments on if follow below.

 First impressions: Patch: The patch applied cleanly and I was able to
 compile your code with no issues.

 Patch: I see your patch removes lines 38 - 40 of python/CMakeLists.txt -
 was this intentional?

 Patch: I think your (c++) code would be better placed under src/analysis
 and become part of the QGIS analysis library. Our intention is to build
 out this library with analytical routines and keep the core and gui libs
 limited to dealing with basic file io, geometry handling, symbolisation
 etc.

 UI: The Auto GCP menu entry is spelled incorrectly (exctration)


 UI: When I first started the agcp plugin, I was prompted for a CRS -
 however there is no way to return to that dialog (that I can see) after
 closing it.

 UI: The icons (while nice) are inconsistent with QGIS icons. QGIS has a
 theming infrastructure which you can make use of or to hedge your bets,
 use icons from or in keeping with the 'GIS' theme since that will most
 likely become the official icon theme at some point in the near future.

 UI: For the map toolbars, a 'zoom to all' icon would be nice.

 UI: The first 3 items under the settings menu probably belong better under
 the 'View' menu.

 Code: Please take a look at the QGIS coding convention docs (supplied as
 text document 'CODING' at the root of the QGIS source tree. Take care of
 indentation (2 spaces) and other such mundane stuff since the code wont be
 accepted into trunk without it. There are tools to automatically format
 your code in a compliant way under the scripts dir (scripts/astyle.sh).
 Check with the mailing list if you need help on using it.

 Documentation: Also take care to add doxygen comments describing each
 public method as can be seen in the other parts of core.

 Documentation: Each C++ source should also get an \ingroup directive (see
 CODING once again). Assuming your code lands up in analysis lib it would
 be something like this:

 {{{
 /** \ingroup analysis
  * The QGis class provides vector geometry analysis functions
  */
 }}}

 UI: Import existing GCP's currently does nothing.

 UI: It would be nice to apply the GCPs to the image to carry out a basic
 georeferencing (easily achieved with a shell call to gdal even) until such
 time as you have written your orthorectification stuff (if that is in
 scope of your project).

 UI: Make use of a qprogressbar to show progress. If the progress is non-
 deterministic, qprogressbar has an option to simply bounce the cursor up &
 down while you are waiting.

 UI: Similar to above, set the cursor to an hour glass.


 These are all mainly housekeeping comments. I was able to generate a few
 simple autgcp runs with the tool using e.g. pan and ms bands from a spot
 image. I still need to do further testing.

 Can you supply me with all your names so that I can credit you - I want to
 write a blog article highlighting your work which will hopefully generate
 more comments and interest (I think many people dont even realise that
 such a thing as Automated GCP collection exists!).

 Keep up the good work!

 Regards

 Tim

-- 
Ticket URL: <http://trac.osgeo.org/qgis/ticket/2987#comment:4>
Quantum GIS <http://qgis.org>
Quantum GIS is an Open Source GIS viewer/editor supporting OGR, PostGIS, and GRASS formats