Add support for KMB files generated with latest MapsMe #8935

strump · 2024-08-12T07:19:24Z

strump commented

2024-08-12 07:19:24 +00:00

Latest MapsMe app upgraded its KMB format version to 9. The easy fix is to parse it the same way as we parse V8 format.

This fix works for some test files. But need to verify with more cases.

TODO: Update unit tests.

Update 2024.09.14

Introduced new version V9MM which inherits V8MM and have little change in tracks format.

Updated unit tests.

Update 2024.09.14

Fixed format of version V9MM to support multi-geometry.

Updated unit tests.

Latest MapsMe app upgraded its KMB format version to `9`. The easy fix is to parse it the same way as we parse V8 format. This fix works for some test files. But need to verify with more cases. TODO: Update unit tests. ### Update 2024.09.14 Introduced new version `V9MM` which inherits `V8MM` and have little change in tracks format. Updated unit tests. ### Update 2024.09.14 Fixed format of version `V9MM` to support multi-geometry. Updated unit tests.

biodranik (Migrated from github.com) reviewed 2024-08-12 23:27:02 +00:00

kml/serdes_binary.hpp

biodranik (Migrated from github.com) commented

2024-08-12 23:27:01 +00:00

Let's add a comment that we likely discover an issue and a need for a separate if later )

biodranik (Migrated from github.com) reviewed 2024-08-13 19:02:52 +00:00

kml/serdes_binary.hpp

biodranik (Migrated from github.com) commented

2024-08-13 19:02:52 +00:00

    // The recent MapsMe update increased the version number, but it is not clear yet what changed/added in a newer version.
    // Revise V9 in case of discovered crashes.
    if (m_header.m_version == Version::V8 || m_header.m_version == Version::V9)

```suggestion // The recent MapsMe update increased the version number, but it is not clear yet what changed/added in a newer version. // Revise V9 in case of discovered crashes. if (m_header.m_version == Version::V8 || m_header.m_version == Version::V9) ```

biodranik (Migrated from github.com) reviewed 2024-08-14 12:28:39 +00:00

biodranik (Migrated from github.com) left a comment

Great, thanks!

kml/header_binary.hpp

					
				@ -26,11 +24,19 @@ enum class Version : uint8_t

				            // tags to kml

				  V9 = 9,   // 01 October 2020: add minZoom to bookmarks

biodranik (Migrated from github.com) commented

2024-08-14 12:21:35 +00:00

  V8MM = 10, // 27 July 2023: MapsMe released version v15.0.71617. Technically its version is 8

```suggestion V8MM = 10, // 27 July 2023: MapsMe released version v15.0.71617. Technically its version is 8 ```

biodranik (Migrated from github.com) commented

2024-08-14 12:21:53 +00:00

  V9MM = 11  // In July 2024 MapsMe released version with a new KMB format. Technically its version is 9

```suggestion V9MM = 11 // In July 2024 MapsMe released version with a new KMB format. Technically its version is 9 ```

kml/kml_tests/serdes_tests.cpp

biodranik (Migrated from github.com) commented

2024-08-14 12:22:36 +00:00

// Can't compare dataFromBinV8.m_categoryData and dataFromBinV8MM.m_categoryData directly
// because new format has less properties and different m_id. Compare some properties here:

```suggestion // Can't compare dataFromBinV8.m_categoryData and dataFromBinV8MM.m_categoryData directly // because new format has less properties and different m_id. Compare some properties here: ```

kml/kml_tests/serdes_tests.cpp

biodranik (Migrated from github.com) commented

2024-08-14 12:23:42 +00:00

    TEST(false, ("Failed to deserialize data from KMB V8 and V9MM", exc.what()));

```suggestion TEST(false, ("Failed to deserialize data from KMB V8 and V9MM", exc.what())); ```

biodranik (Migrated from github.com) commented

2024-08-14 12:24:08 +00:00

    TEST(false, ("Failed to deserialize data from KMB V9MM", exc.what()));

```suggestion TEST(false, ("Failed to deserialize data from KMB V9MM", exc.what())); ```

kml/kml_tests/tests_data.hpp

biodranik (Migrated from github.com) commented

2024-08-14 12:25:21 +00:00

Maybe

// kBinKmlV9MM contains the same bookmarks and tracks as kBinKmlV8

?

Maybe ```suggestion // kBinKmlV9MM contains the same bookmarks and tracks as kBinKmlV8 ``` ?

kml/serdes_binary.hpp

					
				@ -265,4 +273,4 @@

				        m_header.m_eosOffset = m_header.m_stringsOffset;

				        m_header.m_stringsOffset = m_header.m_compilationsOffset;

				      }

				    }

biodranik (Migrated from github.com) commented

2024-08-14 12:26:07 +00:00

Why is it a warning? Maybe info or no log at all?

kml/types_v9mm.hpp

biodranik (Migrated from github.com) commented

2024-08-14 12:26:50 +00:00

  // Extra field introduced in V9MM for tracks.

```suggestion // Extra field introduced in V9MM for tracks. ```

biodranik (Migrated from github.com) commented

2024-08-14 12:27:08 +00:00

// Contains the same sections as FileDataV8MM but with changed m_tracksData format

```suggestion // Contains the same sections as FileDataV8MM but with changed m_tracksData format ```

biodranik (Migrated from github.com) commented

2024-08-14 12:28:20 +00:00

Will

struct TrackDataV9MM : TrackDataV8MM
{
  bool m_flag1 = true;
};

work?

Will ``` struct TrackDataV9MM : TrackDataV8MM { bool m_flag1 = true; }; ``` work?

strump reviewed 2024-08-14 13:10:32 +00:00

kml/types_v9mm.hpp

strump commented

2024-08-14 13:10:32 +00:00

Nope. In DECLARE_VISITOR_AND_DEBUG_PRINT(...) macro we define order of serialization/deserialization of fields. So new field should be mentioned there. But we can ommit

bool operator==(...)
bool operator!=(...)

Those are never used. And inherit TrackData ConvertToLatestVersion() from TrackDataV8MM.

Nope. In `DECLARE_VISITOR_AND_DEBUG_PRINT(...)` macro we define order of serialization/deserialization of fields. So new field should be mentioned there. But we can ommit * `bool operator==(...)` * `bool operator!=(...)` Those are never used. And inherit `TrackData ConvertToLatestVersion()` from TrackDataV8MM.

strump reviewed 2024-08-14 13:18:01 +00:00

kml/serdes_binary.hpp

					
				@ -265,4 +273,4 @@

				        m_header.m_eosOffset = m_header.m_stringsOffset;

				        m_header.m_stringsOffset = m_header.m_compilationsOffset;

				      }

				    }

strump commented

2024-08-14 13:18:01 +00:00

Reduced log level to Info

Reduced log level to `Info`

vng (Migrated from github.com) reviewed 2024-08-14 13:40:33 +00:00

kml/serdes_binary.hpp

vng (Migrated from github.com) commented

2024-08-14 13:40:29 +00:00

This block is equivalent to the previous.
Why do we have all these copy-paste? With new TrackDataV9MM, FileDataV9MM etc?

UPD: Aha, bool for Tracks. Well, ok, but can make a bit cleaner (template FileDataMM)

This block is equivalent to the previous. Why do we have all these copy-paste? With new TrackDataV9MM, FileDataV9MM etc? UPD: Aha, bool for Tracks. Well, ok, but can make a bit cleaner (template FileDataMM)

vng (Migrated from github.com) approved these changes 2024-08-14 13:46:29 +00:00

strump reviewed 2024-08-14 19:01:52 +00:00

kml/types_v9mm.hpp

					
				@ -0,0 +43,4 @@

				    return data;

				  }

				  std::vector<MultiGeometry> m_multiGeometry;

strump commented

2024-08-14 19:01:52 +00:00

By default MultiGeometry struct supports multiple lines. But reader (see visitors.hpp:671 ) always parses only single geometry.

To minimize changes I parse vector<MultiGeometry> each containing a single line. And then merge multiple geometries into in single one.

How to improve it?

@biodranik @vng please verify MultiGeometry mergeGeometry(std::vector<MultiGeometry> aGeometries) function. It could have memory leacks.

By default `MultiGeometry` struct supports multiple lines. But reader (see [visitors.hpp:671](https://github.com/organicmaps/organicmaps/blob/master/kml/visitors.hpp#L671) ) always parses only single geometry. To minimize changes I parse `vector<MultiGeometry>` each containing a single line. And then merge multiple geometries into in single one. How to improve it? @biodranik @vng please verify `MultiGeometry mergeGeometry(std::vector<MultiGeometry> aGeometries)` function. It could have memory leacks.

biodranik (Migrated from github.com) reviewed 2024-08-14 19:36:02 +00:00

biodranik (Migrated from github.com) left a comment

Thanks! @vng how to make it cleaner?

kml/serdes_binary.hpp

biodranik (Migrated from github.com) commented

2024-08-14 19:27:03 +00:00

@vng can you please help and apply a cleaner patch?

kml/types.cpp

biodranik (Migrated from github.com) commented

2024-08-14 19:34:00 +00:00

If m_multigeometry is not needed after merging, then the best way is to avoid unnecessary allocations and copies:

MultiGeometry mergeGeometry(std::vector<MultiGeometry> && geometries)
{
  MultiGeometry merged;
  for (auto && geometry : geometries)
    for (auto && line : geometry.m_lines)
      merged.m_lines.emplace_back(std::move(line));

  geometries.clear();
  return merged;
}

If m_multigeometry is not needed after merging, then the best way is to avoid unnecessary allocations and copies: ```suggestion MultiGeometry mergeGeometry(std::vector<MultiGeometry> && geometries) { MultiGeometry merged; for (auto && geometry : geometries) for (auto && line : geometry.m_lines) merged.m_lines.emplace_back(std::move(line)); geometries.clear(); return merged; } ```