KOSMIndoorMap in kdereview

[Volker Krause]

On Donnerstag, 22. Oktober 2020 21:14:43 CEST Rolf Eike Beer wrote:
> Am Donnerstag, 22. Oktober 2020, 17:25:32 CEST schrieb Volker Krause:
> > Hi,
> > 
> > KOSMIndoorMap is a QML component for showing multi-floor OSM indoor maps
> > (as its very creative name might suggest). It's using maps.kde.org as a
> > data source (same as Marble), and has been created to show interactive
> > maps of train stations for KDE Itinerary.
> > 
> > https://invent.kde.org/libraries/kosmindoormap
> > 
> > KOSMIndoorMap has been growing inside the KPublicTransport repo (due to
> > some building blocks being available there alreay and me being lazy), and
> > has now been split into its own repo. So technically this is coming out
> > of a reviewed repo already rather than from playground, but better be on
> > the safe side :)
> > 
> > The aim is having this (together with KPublicTransport and KDE Itinerary)
> > join the release service for 20.12.
>
> The const version of DataSet::way() returns Way*, but the non-const version
> returns OSM::Way*. I guess the extra qualification is not needed.

fixed

> In OSM2go I use std::unordered_map to store the different types of objects
> which makes lookups much easier. YMMV depending if you want to optimize for
> lookup or memory size.

Since the vectors are sorted, lookup performance there hasn't been an issue so 
far. Lookup performance for tags OTOH has been (and to some extend still is) a 
much bigger issue.

> My recent work there is to get rid of the note, way, and relation prefixes
> or suffixes on function names and make a template from the remaining
> implementation so I don't have to implement the same things 3 times.
> 
> I have derived all 3 object types from a common base class, which allows me
> to simplify things like Element::url() by just calling obj->url() and let a
> virtual overload do the rest. Which is a good example: the final return in
> that function should be replaced by Q_UNREACHABLE(), too.

Right, a common base class and virtual methods would make that code more 
elegant, possibly even making the entire Element/UniqueElement classes 
unnecessary. However, that comes at the cost of an extra sizeof(void*) per 
instance. And given how many instances there are and how aggressively this has 
been optimized for memory size, that would be going in the wrong direction. 
I've actually thought about adding an additional type for the common case of 
tag-less nodes to get rid of the tag storage overhead in that case :)

While this is designed to handle a single (large and detailed) building only, 
it can actually handle an entire city as well by now, without any tiling. That 
should leave us enough room even in case of an exploding level of detail in 
large train stations, and for resource-constraint phones.

Q_UNREACHABLEs have been added.

> The nodes vectors in Element::outerPath() could benefit from a call to
> reserve().

appendNodesFromWay() (and via that also assemblePath()) do call reserve on 
those vectors internally.

> The coding style for the remark-if in XmlParser::parse() is inconsistent.
> Or, if compared to the rest of the file, it's the only consistent one in
> that function.

fixed

> Eike

Thanks for the feedback!

Volker