Re: KOSMIndoorMap in kdereview
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
Re: KOSMIndoorMap in kdereview
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=) > > > 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? Cheers, Albert > > 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 > > > >
Re: KOSMIndoorMap in kdereview
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. > 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. 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
Re: KOSMIndoorMap in kdereview
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
Re: KOSMIndoorMap in kdereview
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.) 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; Should ./bin/indoormap do anything? I have one empty combo and one combo that lets me change the style. You have one qsTr in the qml that it's not going to work, also a "TODO i18n?" that i would say yes? It has tests \o/ the tests don't pass /o\ https://pastebin.com/CFcdVJHh Cheers, Albert > Thanks, > Volker > > > >
Re: KOSMIndoorMap in kdereview
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. 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. 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. The nodes vectors in Element::outerPath() could benefit from a call to reserve(). 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. Eike signature.asc Description: This is a digitally signed message part.
