[Marble-devel] Review Request: Kothic Json parser for vector data tiles

Tue Aug 14 10:44:52 UTC 2012

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/106008/#review17364
-----------------------------------------------------------

Looks good, just nitpicking again.

src/plugins/runner/json/JsonParser.h
<http://git.reviewboard.kde.org/r/106008/#comment13565>

    please prefix include guards with MARBLE_, i.e. MARBLE_JSONPARSER_H here

src/plugins/runner/json/JsonParser.h
<http://git.reviewboard.kde.org/r/106008/#comment13566>

    Why virtual? I don't think any other class derives from it, does it?

src/plugins/runner/json/JsonParser.cpp
<http://git.reviewboard.kde.org/r/106008/#comment13567>

    m_data = 0; should not be needed.

    For consistency however,

    delete m_document;

    should be there (in case nobody called releaseDocument());

src/plugins/runner/json/JsonParser.cpp
<http://git.reviewboard.kde.org/r/106008/#comment13569>

    This isn't mentioned in the API comment, so I'd rather call 
    delete m_document;
    here instead of the assertion.

src/plugins/runner/json/JsonParser.cpp
<http://git.reviewboard.kde.org/r/106008/#comment13568>

    Given it's single use, I'd just write

    m_document = new GeoDataDocument;

    and remove the createDocument() method.

src/plugins/runner/json/JsonParser.cpp
<http://git.reviewboard.kde.org/r/106008/#comment13570>

    Doesn't this need to be initialized to 0 to avoid a memory leak here? The next use of placemark in the for loop creates another GeoDataPlacemark.

src/plugins/runner/json/JsonParser.cpp
<http://git.reviewboard.kde.org/r/106008/#comment13571>

    Are those sane default values? If not, should the bbox check below abort parsing if it fails?

src/plugins/runner/json/JsonParser.cpp
<http://git.reviewboard.kde.org/r/106008/#comment13572>

    Curly brackets are missing, should always be there even for one-liners.

src/plugins/runner/json/JsonParser.cpp
<http://git.reviewboard.kde.org/r/106008/#comment13573>

    Shouldn't that be coors.size()-2? You're increasing x by 2 below in each iteration.

src/plugins/runner/json/JsonParser.cpp
<http://git.reviewboard.kde.org/r/106008/#comment13574>

    When does that (auxX=0 || auxY=0) happen, why is it an error? (0,*) and (*,0) are valid coordinates after all.

src/plugins/runner/json/JsonParser.cpp
<http://git.reviewboard.kde.org/r/106008/#comment13575>

    Here happens a lot of uneeded copy operations (to append a coordinate, each time the existing ring is copied, the coordinate is appended and the ring is copied again). The copies would be avoided when creating the ring before the for loop is started and setting the outer boundary after the for loop (which fills the ring) is completed.

src/plugins/runner/json/JsonParser.cpp
<http://git.reviewboard.kde.org/r/106008/#comment13576>

    Why is this not done above where the other ones (polygon, linestring) are initialized? If it can happen repeatedly, this would create a memory leak and would have to be handled differently.

src/plugins/runner/json/JsonPlugin.h
<http://git.reviewboard.kde.org/r/106008/#comment13577>

    MARBLE_JSONPLUGIN_H

src/plugins/runner/json/JsonRunner.h
<http://git.reviewboard.kde.org/r/106008/#comment13578>

    MARBLE_JSONRUNNER_H

src/plugins/runner/json/JsonRunner.cpp
<http://git.reviewboard.kde.org/r/106008/#comment13579>

    No need for the complicated static_cast conversion. Just do a

    GeoDataDocument* document = parser.relaseDocument();
    document->setDocumentRole( role );

    and remove the lines involving doc, emit the document.

- Dennis Nienhüser

On Aug. 14, 2012, 9:29 a.m., Ander Pijoan wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/106008/
> -----------------------------------------------------------
> 
> (Updated Aug. 14, 2012, 9:29 a.m.)
> 
> 
> Review request for Marble.
> 
> 
> Description
> -------
> 
> During GSoC 2012 for vector tile rendering, Kothic's servers json tiles (hosted on http://osmosnimki.ru/vtile/ - please don't use it heavily-loaded projects for now) have been used while OpenStreetMap finishes building its own data tile server. These Kothic tiles have a very similar format (https://github.com/kothic/kothic-js/wiki/Tiles-format) to the ones that OSM will have.
> 
> A problem was found with QScriptValue for multidimensional arrays. When having for example [[20,10],[20,30].[30,30]] if we extract the property it will flatten to 20,10,20,30,30,30. This parser doesn't create separate inner geometries because of this. It will connect the inner geometry with its outer.
> 
> It will set the geometries name and style according to what OsmVisualCategory generates for the parsed tags.
> 
> This parser creates a GeoDataDocument with all the extracted data.
> 
> 
> Diffs
> -----
> 
>   src/plugins/runner/CMakeLists.txt 4ab295a 
>   src/plugins/runner/json/CMakeLists.txt PRE-CREATION 
>   src/plugins/runner/json/JsonParser.h PRE-CREATION 
>   src/plugins/runner/json/JsonParser.cpp PRE-CREATION 
>   src/plugins/runner/json/JsonPlugin.h PRE-CREATION 
>   src/plugins/runner/json/JsonPlugin.cpp PRE-CREATION 
>   src/plugins/runner/json/JsonRunner.h PRE-CREATION 
>   src/plugins/runner/json/JsonRunner.cpp PRE-CREATION 
> 
> Diff: http://git.reviewboard.kde.org/r/106008/diff/
> 
> 
> Testing
> -------
> 
> 
> Screenshots
> -----------
> 
> Kothic Json Parser
>   http://git.reviewboard.kde.org/r/106008/s/676/
> 
> 
> Thanks,
> 
> Ander Pijoan
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/marble-devel/attachments/20120814/851e29d2/attachment-0001.html>