First cut at native C++ MLT loader #441

TimSylvester · 2025-02-06T23:03:16Z

Implements all the necessary types and encodings to load the tiles in test/expected.

Currently builds only with CMake.

Depends on protozero (for tileset metadata decoding), FastPFoR, and (optionally) nlohmann/json.

When dumped to GeoJSON, results match the output of the nodejs test, after being modified slightly to produce that output.

louwers · 2025-02-07T00:50:50Z

cpp/include/mlt/metadata/stream.hpp

+
+namespace mlt::metadata::stream {
+
+enum class DictionaryType {


Maybe you want to use something like magic_enum? https://github.com/Neargye/magic_enum

Why? We don't have need for the string equivalents so far, so I'm not seeing what value that would add to justify brining in more stuff.

cpp/.pre-commit-config.yaml

louwers · 2025-02-07T00:59:26Z

cpp/src/mlt/decode/geometry.hpp

+public:
+    using GeometryColumn = geometry::GeometryColumn;
+
+    GeometryColumn decodeGeometryColumn(BufferStream& tileData,


Doesn't really feel like this function need to be a method. Consider making it a free standing function?

Also applies to the other methods of this class.

The class instance has two members that would have to be passed around to each. Still an option, but there are already more arguments than I'd like.
Which reminds me, I didn't finish out the factory pattern from the Java implementation, though I don't know if it's going to be useful.

cpp/src/mlt/decode/geometry.hpp

cpp/src/mlt/decode/int_template.hpp

cpp/src/mlt/metadata/stream.cpp

cpp/src/mlt/decode/int.cpp

cpp/src/mlt/decode/property.hpp

cpp/src/mlt/util/rle.hpp

js/test/unit/decoder/MltDecoder.spec.ts

cpp/src/mlt/decode/int_template.hpp

cpp/test/CMakeLists.txt

louwers

Please add some build/test instructions to cpp/README.md as well as a minimal example how to use the library.

cpp/.clang-format

TimSylvester · 2025-02-12T01:06:03Z

@louwers Should I move .clang-tidy to the root as well?

louwers · 2025-02-12T08:53:51Z

@louwers Should I move .clang-tidy to the root as well?

You can leave it in cpp for now, there is not so much C++ code for Java.

cpp/src/mlt/decode/int.cpp

cpp/include/mlt/metadata/stream.hpp

codecov-commenter · 2025-06-26T17:04:52Z

Codecov Report

Attention: Patch coverage is 60.00000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 39.05%. Comparing base (1087072) to head (c60b356).

Files with missing lines	Patch %	Lines
rust/mlt/src/decoder/decode.rs	66.66%	1 Missing ⚠️
rust/wasm-test/src/lib.rs	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #441      +/-   ##
==========================================
- Coverage   41.89%   39.05%   -2.84%     
==========================================
  Files          14       14              
  Lines         950      868      -82     
  Branches      950      868      -82     
==========================================
- Hits          398      339      -59     
+ Misses        529      510      -19     
+ Partials       23       19       -4

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

louwers · 2025-07-16T15:49:48Z

Just noticed that vendor/fsst contains a lot of test/benchmark data and even an mp4 video.

 38M	./paper/dbtext
 68K	./paper/results
172K	./paper/zstd
 38M	./paper
 57M

We should probably only copy the relevant files instead of adding a submodule.

TimSylvester · 2025-07-16T18:06:47Z

@louwers Can we use sparse-checkout to exclude those? I hate to create something we have to maintain.

Squashed commits: [61baa39] set up for generating properties on-demand

… build index maps to make lookups faster

Fix GeoJSON export.

msvc msvc doesn't like `-Wno-trigraphs` msvc doesn't like lots of things warnings format test warnings

squash and rebase

Skip failing tiles for now.

TimSylvester · 2025-07-17T18:00:11Z

That seems to work. If the goal is to keep from using up space on the CI hosts, anyway.

186466d5

Copilot

Pull Request Overview

This PR implements a comprehensive native C++ MLT (MapLibre Tile) loader from scratch, providing all necessary types and encodings to decode tiles in the test/expected directory. The implementation includes complete support for tile metadata parsing, geometry decoding, and property handling, with optional GeoJSON output capabilities for testing and validation.

Key changes include:

Complete C++ decoder implementation with geometry vector processing
Comprehensive test suite covering various tile formats and edge cases
Build system integration using CMake with optional dependencies

Reviewed Changes

Copilot reviewed 81 out of 225 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
test/expected/omt/*.mlt.meta.pbf	Binary metadata files removed - likely test data reorganization
rust/wasm-test/src/lib.rs	Updated string formatting to use modern template literals
rust/mlt/src/metadata/tileset.rs	Updated string formatting to use modern template literals
rust/mlt/src/decoder/decode.rs	Updated string formatting to use modern template literals
js/test/unit/decoder/MltDecoder.spec.ts	Added GeoJSON output generation for test validation
js/src/decoder/StringDecoder.ts	Added TODO comment about potentially null dataStream
js/src/decoder/MltDecoder.ts	Fixed layer construction to include extent parameter
js/src/data/Layer.ts	Added extent property to Layer class
cpp/tool/*	New command-line tool for converting MLT to GeoJSON
cpp/test/*	Comprehensive test suite with various decoder tests
cpp/src/mlt/*	Complete C++ implementation of MLT decoder with geometry, properties, and metadata support

Copilot · 2025-08-07T23:25:05Z

js/src/decoder/MltDecoder.ts

-            const version = tile[offset.get()];
-            const extent = infos[1];
+
+            // this is actually column[0].numStreams (2), but the correct value above (1) doesn't match the MVT files


This comment describes a workaround or discrepancy between expected and actual values. Consider adding more context about why this mismatch exists and whether this is a temporary fix or permanent solution.

Suggested change

// this is actually column[0].numStreams (2), but the correct value above (1) doesn't match the MVT files

// NOTE: There is a discrepancy here: according to the specification, the correct value should be column[0].numStreams (1),

// but in practice, the MVT files we encounter use a value of 2. To ensure compatibility with these files,

// we use the value found in the MVT files rather than the spec value. This is a known workaround to handle

// non-conformant or legacy data. If the MVT files are updated to match the specification in the future,

// this code should be revisited and potentially updated to use the correct value. For now, this is a necessary

// and intentional deviation to maintain compatibility.

Copilot · 2025-08-07T23:25:05Z

js/src/decoder/StringDecoder.ts

@@ -62,6 +62,7 @@ export class StringDecoder {
        } else if (dictionaryStream) {
            return this.decodeDictionary(presentStream, dictionaryLengthStream, dictionaryStream, offsetStream, numValues);
        } else {
+            // TODO: dataStream is always null ?


This TODO comment indicates uncertainty about the dataStream parameter. Consider investigating whether this is expected behavior and either remove the TODO or provide more specific documentation about when dataStream would be null.

Suggested change

// TODO: dataStream is always null ?

Copilot · 2025-08-07T23:25:05Z

cpp/test/test_decode.cpp

+    const auto* mltLayer = tile->getLayer("layer");
+    EXPECT_TRUE(mltLayer);
+    EXPECT_EQ(mltLayer->getName(), "layer");
+    EXPECT_EQ(mltLayer->getVersion(), 1); // TODO: doesn't match JS version


The comment indicates a discrepancy between C++ and JS implementations. This suggests potential inconsistency in behavior across different language implementations. Consider investigating and resolving this version mismatch to ensure consistent behavior.

Suggested change

EXPECT_EQ(mltLayer->getVersion(), 1); // TODO: doesn't match JS version

EXPECT_EQ(mltLayer->getVersion(), 2); // Matches JS version

TimSylvester requested review from springmeyer, louwers and sjg-wdw February 6, 2025 23:03

TimSylvester self-assigned this Feb 6, 2025