partial reimplementation of MvtWriter to support feature writing and outputting layers #264

styrowolf · 2025-09-01T22:09:44Z

I agree to follow the project's code of conduct.
I added an entry to the project's change log file if knowledge of this change could be valuable to users.
- Usually called CHANGES.md or CHANGELOG.md
- Prefix changelog entries for breaking changes with "BREAKING: "

This is an implementation of feature writing for MVTs, as discussed in #263. This PR builds upon @nyurik's previous WIP reimplementation #181. It passes all of the mvt_writer tests. I am not sure if I understood the API exactly correctly but this seems to be a pretty good starting point, as individual feature/geom can be converted to a MVT feature with the ToMvt trait as well as MvtWriter can process multiple feature and output them to a layer.

Here is an example of feature writing and layer output with MvtWriter (from GeoJSON to MVT tiles PR, maplibre/martin#2098)

Let me know how we can move forward with this PR.

layer writing to MvtWriter

whether to scale

styrowolf · 2025-09-02T19:45:08Z

@urschrei the checks fail on something unrelated to the PR. when I run cargo clippy --workspace --all-features --bins --examples --tests --benches -- -D warnings on my machine, it passes. do I need to do anything?

michaelkirk · 2025-09-02T19:58:22Z

It's kind of annoying when new clippy lints break builds, but I guess it's a forcing function to improve (usually they're improvements anyway 😆) the codebase.

If you want to speed things along, you can fix the clippy lints in a separate PR.

kylebarron · 2025-09-03T17:23:38Z

I made #265 to fix latest clippy

styrowolf · 2025-09-04T13:35:53Z

Could someone review this PR when possible, so that we can move forward? Appreciate the help.

geozero/src/mvt/mod.rs

geozero/src/mvt/mvt_writer.rs

kylebarron · 2025-09-04T15:00:42Z

geozero/src/mvt/mvt_writer.rs

+    tags_builder: TagsBuilder,
+}
+
+impl Default for MvtWriter {


Does it make sense to implement Default here? The arguments to new (extent: u32, left: f64, bottom: f64, right: f64, top: f64) don't to me seem to have a natural default.

The default call creates an "unscaled" MvtWriter, where the geometry is assumed to be in tile coordinates and accepted as is. The new call has arguments to scale geometries. Maybe we could rename this or clarify that the Default is an unscaled writer.

I still think this would be clearer to not be implemented as Default. Default should be painfully obvious what it does, and I don't think this meets that bar.

Just adding new_unscaled as a new constructor isn't enough in my opinion because the Default impl is still public

geozero/src/mvt/tile_value.rs

geozero/src/mvt/mvt_writer.rs

geozero/src/mvt/tile_value.rs

styrowolf · 2025-09-05T11:23:17Z

Current questions

Should we encode ColumnValue::Binary as base64 string in TileValue? partial reimplementation of MvtWriter to support feature writing and outputting layers #264 (comment)
Are the changes in MvtWriter and its implementation of Default ready to go? Is there any concerns I've not addressed or something that should be changed?
Any docs, examples, explanations to add?

Thank you for your time and your reviews.

frewsxcv

I don't have any other blocking comments here, but it'd be great to get sign off from @michaelkirk and @kylebarron as well

kylebarron · 2025-09-05T21:39:31Z

Should we encode ColumnValue::Binary as base64 string in TileValue? partial reimplementation of MvtWriter to support feature writing and outputting layers #264 (comment)

I think there should at least be an option to do so. It's probably the most straightforward way of encoding Binary properties, right?

Are the changes in MvtWriter and its implementation of Default ready to go? Is there any concerns I've not addressed or something that should be changed?

I think it would be clearer if it were a separate constructor with an associated docstring. To me, Default should be painfully obvious what will happen, and it's not in this case.

Any docs, examples, explanations to add?

I think having a runnable and doctested example would be ideal here. It can be small

comments clarifying MvtWriter

styrowolf · 2025-09-06T21:25:17Z

I just added base64 encoding for ColumnValue and MvtWriter docstrings. Feel free to take a look. I'll add the runnable, doctested example when I find the time. Thanks

styrowolf · 2025-09-07T06:45:04Z

I've added the doc-tested example as well. Ready for (hopefully!) final review.

nyurik

why are we removing integration tests here? Are they fundamentally wrong now? Also, made a few minor comments. Thank you for working on this!

geozero/src/mvt/mvt_writer.rs

geozero/Cargo.toml

Default clearer

styrowolf · 2025-09-08T06:22:14Z

I added the integration tests back in. Should be good to go.

styrowolf · 2025-09-10T08:45:20Z

There seems to be only one blocker for this PR (link to comment):

Just adding new_unscaled as a new constructor isn't enough in my opinion because the Default impl is still public

What can I do to remove this blocker?

kylebarron · 2025-09-10T16:19:25Z

geozero/src/mvt/mvt_writer.rs

+    /// The resulting writer expects all geometries to be provided in tile coordinate space,
+    /// matching the specified `extent`.
+    pub fn new_unscaled(extent: u32) -> MvtWriter {
+        assert_ne!(extent, 0);


I don't think we should panic here. Either we should allow this function to return a Result, or we should make the parameter NonZeroU32

kylebarron · 2025-09-10T16:20:08Z

Just adding new_unscaled as a new constructor isn't enough in my opinion because the Default impl is still public

What can I do to remove this blocker?

My opinion there would be that we should remove the Default impl because it's not obvious what it does. And have only new and new_unscaled constructors.

of a panic

styrowolf · 2025-09-10T19:15:12Z

I removed the default impl and changed constructors so that they return Results. I also updated the changelog with the breaking changes.

nyurik · 2025-09-10T19:35:48Z

looks good, thanks!

nyurik and others added 9 commits November 26, 2023 23:03

wip

6b1785f

expose mvt internals

f27cf3e

Merge remote-tracking branch 'nyurik/mvt-wrt-prop' into mvt-reimpl, add

94d8b02

layer writing to MvtWriter

pass all mvt_writer tests except mvt::mvt_writer::test::geo_to_mvt

d42ab5a

clarify testcase fail

c41a7cb

use bool (instead of setting extent to 0) in MvtWriter in deciding

17c33bb

whether to scale

revert exposing mvt_commands internals

5a9dbf2

fix TagBuilder doctest

861e8fe

cargo fmt

66fedaf

styrowolf mentioned this pull request Sep 2, 2025

feat(geojson): add mvt tiles generation from GeoJSON maplibre/martin#2098

Draft

Merge remote-tracking branch 'origin/main' into mvt-reimpl

071d199

kylebarron reviewed Sep 4, 2025

View reviewed changes

restore ToMvt impl. over GeozeroGeometry

8a4849c

frewsxcv reviewed Sep 4, 2025

View reviewed changes

geozero/src/mvt/mvt_writer.rs Outdated Show resolved Hide resolved

michaelkirk reviewed Sep 4, 2025

View reviewed changes

geozero/src/mvt/mvt_writer.rs Outdated Show resolved Hide resolved

geozero/src/mvt/tile_value.rs Outdated Show resolved Hide resolved

revert geo_to_mvt test to use geo_types, erase extraneous comments

3e11224

frewsxcv approved these changes Sep 5, 2025

View reviewed changes

Convert ColumnValue::Bytes to TileValue::Str using base64 encoding,

f712c0d

comments clarifying MvtWriter

styrowolf added 5 commits September 6, 2025 23:36

fix clippy error

9226922

restore geo_screen_coords_to_mvt test

f224384

reword MvtWriter comments

9ac39ea

add doctested example for MvtWriter

fde78d2

update changelog

f8e6dae

nyurik reviewed Sep 8, 2025

View reviewed changes

geozero/src/mvt/mvt_writer.rs Outdated Show resolved Hide resolved

geozero/src/mvt/mvt_writer.rs Outdated Show resolved Hide resolved

geozero/src/mvt/mvt_writer.rs Outdated Show resolved Hide resolved

geozero/Cargo.toml Outdated Show resolved Hide resolved

styrowolf added 2 commits September 8, 2025 08:18

put back integration tests, move dep to workspace Cargo.toml, impl

50f6a7a

Default clearer

add traits for tests to compile

55c1e0e

kylebarron reviewed Sep 10, 2025

View reviewed changes

styrowolf added 2 commits September 10, 2025 21:11

remove Default impl for MvtWriter, make nonzero extent an error instead

658bd2d

of a panic

update changelog with breaking changes

356aba8

nyurik approved these changes Sep 10, 2025

View reviewed changes

nyurik added this pull request to the merge queue Sep 10, 2025

Merged via the queue into georust:main with commit 11525cf Sep 10, 2025
1 check passed

styrowolf deleted the mvt-reimpl branch September 10, 2025 20:59

partial reimplementation of MvtWriter to support feature writing and outputting layers #264

partial reimplementation of MvtWriter to support feature writing and outputting layers #264

Uh oh!

Conversation

styrowolf commented Sep 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

styrowolf commented Sep 2, 2025

Uh oh!

michaelkirk commented Sep 2, 2025

Uh oh!

kylebarron commented Sep 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

styrowolf commented Sep 4, 2025

Uh oh!

Uh oh!

Uh oh!

kylebarron Sep 4, 2025

Choose a reason for hiding this comment

Uh oh!

styrowolf Sep 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kylebarron Sep 8, 2025

Choose a reason for hiding this comment

Uh oh!

kylebarron Sep 8, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

styrowolf commented Sep 5, 2025

Uh oh!

frewsxcv left a comment

Choose a reason for hiding this comment

Uh oh!

kylebarron commented Sep 5, 2025

Uh oh!

styrowolf commented Sep 6, 2025

Uh oh!

styrowolf commented Sep 7, 2025

Uh oh!

nyurik left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

styrowolf commented Sep 8, 2025

Uh oh!

styrowolf commented Sep 10, 2025

Uh oh!

kylebarron Sep 10, 2025

Choose a reason for hiding this comment

Uh oh!

kylebarron commented Sep 10, 2025

Uh oh!

styrowolf commented Sep 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nyurik commented Sep 10, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

styrowolf commented Sep 1, 2025 •

edited

Loading

kylebarron commented Sep 3, 2025 •

edited

Loading

styrowolf Sep 4, 2025 •

edited

Loading

styrowolf commented Sep 10, 2025 •

edited

Loading