KOSMIndoorMap in kdereview

[Volker Krause]

On Freitag, 23. Oktober 2020 22:29:39 CEST Albert Astals Cid wrote:
> El divendres, 23 d’octubre de 2020, a les 16:56:37 CEST, Volker Krause va 
escriure:
> > On Freitag, 23. Oktober 2020 00:45:46 CEST Albert Astals Cid wrote:
> > > El dijous, 22 d’octubre de 2020, a les 17:25:32 CEST, Volker Krause va
> > 
> > escriure:
> > > > 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.
> > > 
> > > Is this fixable?
> > > 
> > > [libprotobuf WARNING google/protobuf/compiler/parser.cc:648] No syntax
> > > specified for the proto file: osmformat.proto. Please use 'syntax =
> > > "proto2";' or 'syntax = "proto3";' to specify a syntax version.
> > > (Defaulted
> > > to proto2 syntax.) [libprotobuf WARNING
> > > google/protobuf/compiler/parser.cc:648] No syntax specified for the
> > > proto
> > > file: fileformat.proto. Please use 'syntax = "proto2";' or 'syntax =
> > > "proto3";' to specify a syntax version. (Defaulted to proto2 syntax.)
> > 
> > fixed, although that requires modifying the 3rdparty proto files
> > 
> > > and this?
> > > 
> > > style/mapcsslexer.l:65: warning, no es pot satisfer la regla
> > > style/mapcssparser.y:143.11-14: advertiment: valor sense usar: $2
> > > [-Wother]
> > > 
> > >   143 | | Ruleset Rule
> > >   
> > >       |           ^~~~
> > > 
> > > and this?
> > > 
> > > src/map/scene/scenegeometry.cpp: In function ‘double
> > > KOSMIndoorMap::SceneGeometry::polylineMidPointAngle(const QPolygonF&)’:
> > > src/map/scene/scenegeometry.cpp:75:24: warning: unused variable ‘r’
> > > [-Wunused-variable] 75 |             const auto r = ((length + l) -
> > > lineLength / 2.0) / l;
> > 
> > both fixed
> > 
> > > Should ./bin/indoormap do anything? I have one empty combo and one combo
> > > that lets me change the style.
> > 
> > Good point, I've documented the test apps in the README now. Try
> > `indoormap -c 52.52512,13.36966` or alternatively `qmlscene
> > tests/indoormap.qml` to see a few more features (after updating to latest
> > master to make this work with your locale too, see below).
> > 
> > > You have one qsTr in the qml that it's not going to work, also a "TODO
> > > i18n?" that i would say yes?
> > 
> > Fixed, leftovers from before switching to ki18n.
> 
> Since i understand this is a library, you want to use i18nd on qml (you're
> using that on your c++ by using -DTRANSLATION_DOMAIN=)

Indeed, fixed.

> > > It has tests \o/ the tests don't pass /o\ https://pastebin.com/CFcdVJHh
> > 
> > Ouch, you actually found some quite serious breakage that I had entirely
> > missed since neither my local nor the CI locale is affected... This all
> > goes down to some string to floating point number conversion being done
> > locale- aware while they shouldn't be.
> 
> Glad to be of service :)
> 
> Another thing, do you want d-pointers to make keeping ABI easier or don't
> care at this point?

The one class of the C++ interface that is actually used externally at this 
point has a d-pointer already, everything else that is exported is for the QML 
plugins and unit tests in the same repo. This might change over time though, 
at which point more d-pointers would indeed become necessary.

This wont work for the low-level OSM primitives though (that's the points, 
lines and polygons making up the map geometry). Those exists in so large 
quantities that the additional allocation overhead becomes prohibitive.

The server-side tile generator is using Marble which has d-pointers on those 
types, last time I measured that about 10% of the total runtime was spent just 
on d-pointer destruction. Construction/allocation will likely cost at least as 
much as well, but that's harder to separate in the measurements from things 
that have to happen either way.

Regards,
Volker

> > Dirty fixes applied, once std::from_chars becomes available for floating
> > point types this can be addressed more cleanly.
> > 
> > We should run the CI with rotating/random locales :)
> > 
> > Thanks for the feedback!
> > Volker