core: add Rust-backed MLT tile data#4428
Conversation
for more information, see https://pre-commit.ci
…impl # Conflicts: # src/mbgl/tile/vector_mlt_tile_data.cpp
… vector-mlt-tile-data-impl
for more information, see https://pre-commit.ci
… vector-mlt-tile-data-impl
Bloaty Results 🐋Compared to main Full report: https://maplibre-native.s3.eu-central-1.amazonaws.com/bloaty-results/pr-4428-compared-to-main.txtCompared to d387090 (legacy) Full report: https://maplibre-native.s3.eu-central-1.amazonaws.com/bloaty-results/pr-4428-compared-to-legacy.txt |
|
Benchmark Results ⚡ Full report: https://maplibre-native.s3.eu-central-1.amazonaws.com/benchmark-results/pr-4428-compared-to-main.txt |
CommanderStorm
left a comment
There was a problem hiding this comment.
Mostly LGTM, a few aspects are a bit weird though.
How is perf compared to cpp?
| #[derive(Clone, Copy, PartialEq, Eq)] | ||
| enum MltValueType { | ||
| Missing = 0, | ||
| Null = 1, | ||
| Bool = 2, | ||
| I64 = 3, | ||
| U64 = 4, | ||
| F32 = 5, | ||
| F64 = 6, | ||
| String = 7, | ||
| } | ||
|
|
||
| #[derive(Clone, Copy)] | ||
| struct MltValue { | ||
| typ: MltValueType, | ||
| bool_value: bool, | ||
| i64_value: i64, | ||
| u64_value: u64, | ||
| f32_value: f32, | ||
| f64_value: f64, | ||
| } |
There was a problem hiding this comment.
A bit sad that there is no simpler way to do this.
| #[derive(Clone, Copy)] | ||
| struct MltFeatureId { | ||
| has_id: bool, | ||
| value: u64, | ||
| } |
There was a problem hiding this comment.
Interesting that this is not yet implement in cxx
|
|
||
| if (!testCase.fastPFOREnabled && testCase.useFastPFOR) { | ||
| // If fast PFOR is not enabled, the fast PFOR tile should fail to parse | ||
| if (!testCase.fastPFOREnabled && testCase.useFastPFOR && layerNames.empty()) { |
There was a problem hiding this comment.
If layerNames is empty, how can size be anything but zero?
Is this still testing something?
| GeometryCollection(const GeometryCollection&) = default; | ||
|
|
||
| std::shared_ptr<const mlt::MapLibreTile> triangleOwner; | ||
| std::shared_ptr<const void> triangleOwner; |
There was a problem hiding this comment.
Is this still save?
What is const void here?
Why do we need to keep it?
| std::string toStdString(rust::String value) { | ||
| return {value.data(), value.size()}; | ||
| } |
There was a problem hiding this comment.
rust::String implements the explicit converter constructor (not sure what the name is), so you can just std::string(value).
https://cxx.rs/binding/string.html
Also applies to below rust::Str.
Also, Inline afterwards ^^
| VectorMLTTileData::VectorMLTTileData(std::shared_ptr<const std::string> data, bool) | ||
| : impl(std::make_shared<Impl>(std::move(data))) {} | ||
|
|
||
| VectorMLTTileData::VectorMLTTileData(const VectorMLTTileData&) = default; | ||
| VectorMLTTileData::VectorMLTTileData(VectorMLTTileData&&) noexcept = default; | ||
| VectorMLTTileData::~VectorMLTTileData() = default; | ||
|
|
||
| std::unique_ptr<GeometryTileData> VectorMLTTileData::clone() const { | ||
| return std::make_unique<VectorMLTTileData>(*this); | ||
| } | ||
|
|
||
| std::unique_ptr<GeometryTileLayer> VectorMLTTileData::getLayer(const std::string& name) const { | ||
| return impl->getLayer(name); | ||
| } | ||
|
|
||
| std::vector<std::string> VectorMLTTileData::layerNames() const { | ||
| return impl->layerNames(); | ||
| } |
There was a problem hiding this comment.
Should they not also be in the class? Why are they below the class?
| .collect(), | ||
| property_values: Vec::new(), | ||
| ffi_property_values: Vec::new(), | ||
| part_offsets: vec![0], |
There was a problem hiding this comment.
This will mean we always alloc here, can we make the first index implied 0 in the cpp code or in the decoding instead?
I assume this is reasonably hot
| } | ||
|
|
||
| fn value(typ: ffi::MltValueType) -> ffi::MltValue { | ||
| ffi::MltValue { |
There was a problem hiding this comment.
nit: I'd inline these, derive Default for the last and .. Default::default(), but that is likely fine too.
| f64_value: *f64_value, | ||
| ..value(ffi::MltValueType::F64) | ||
| }, | ||
| MltPropertyValue::String(_) => value(ffi::MltValueType::String), |
There was a problem hiding this comment.
Is this what we want? Where is the string stored?
| let Some(start) = layer.part_offsets.get(offset_index) else { | ||
| return 0; | ||
| }; | ||
| let Some(end) = layer.part_offsets.get(offset_index + 1) else { | ||
| return 0; |
There was a problem hiding this comment.
Not 💯 sure if this compiles, but my intuition would be to do this.
| let Some(start) = layer.part_offsets.get(offset_index) else { | |
| return 0; | |
| }; | |
| let Some(end) = layer.part_offsets.get(offset_index + 1) else { | |
| return 0; | |
| let Some([start,end]) = layer.part_offsets.get(offset_index..offset_index +1) else { | |
| return 0; | |
| }; |
About the same. Regarding your Rust comments, this was 100% vibe coded as a soft of proof of concept. I don't really know Rust. |
Bloaty Results (iOS) 🐋Compared to main Full report: https://maplibre-native.s3.eu-central-1.amazonaws.com/bloaty-results-ios/pr-4428-compared-to-main.txt |
The Rust MLT decoder has a safe FastPFor implementation.
This PR is a vibe coded (GPT 5.6) integration with
mlt-core.