search

wdtinc / mapbox-vector-tile-java

Java Mapbox Vector Tile Library for Encoding/Decoding
Apache License 2.0
147 stars 73 forks source link

add layer support #8

Closed snodnipper closed 6 years ago

snodnipper commented 6 years ago

Improve layer support within the vector tile library.

I am not precious about any of this code and I am happy to refactor as appropriate - for example, it might be helpful to rename LayerGroup as Mvt.

Thanks!

ShibaBandit commented 6 years ago

Thank's for the pull request, I'll check this out some more soon.

ShibaBandit commented 6 years ago

I'm seeing the needs behind this pull request now. It may not be possible to get the MVT metadata like the layer name + other metadata back out of a JTS tile layers. I'm not sure I understand the 'Sink' private interface change yet. Using a private interface in a public API method likely an issue. I may provide some edits if that's alright.

ShibaBandit commented 6 years ago

Is the use of a Sink an attempt to mix in some reactive (Rx) programming style? Maybe this could be taken out and put into a separate pull request. I have some concerns about it, especially where every geometry involves a layer name lookup (linear search layer name string comparison?).

ShibaBandit commented 6 years ago

One super picky comment - I generally format code with IntelliJ - auto-indent so function parameters line up. Some new functions in MVTReader have misaligned parameters.

snodnipper commented 6 years ago

thanks for the comments.

1) Sink interface - I only created this to reduce code duplication and maintain existing code contracts. There are two slightly different code paths that could be simplified (but would be a breaking change).

loadMvt(is, geomFactory, tagConverter, ringClassifier, (layerName, geometry) -> tileGeoms.add(geometry));

and

loadMvt(is, geomFactory, tagConverter, ringClassifier, (layerName, geometry) -> { Layer layer = layerGroup.getLayer(layerName); if (layer == null) { layer = new Layer(layerName); layerGroup.addLayer(layer); } layer.add(geometry); });

imho, that complexity could be designed out by writing loadMvt to only use Layers...the code block is:

        for(VectorTile.Tile.Layer nextLayer : mvt.getLayersList()) {
            String layerName = nextLayer.getName();

We wouldn't therefore need to continually perform lookups for the layer.

I agree that the public / private aspect of void loadMvt was confusing - I have now marked it as private.

2) Code formatting - no problem, I hope to look at that tomorrow. I normally use checkstyle rules and have been working with IntelliJ in a completely unknown state.

I have added you to our repo should you want to commit further on this branch. No problem either way but I can stop amending commits if we are both working on the branch.

snodnipper commented 6 years ago

I am currently working on a "feature/layer-enhancement" branch to deal with the inefficiencies discussed - I will message when uploaded for further discussion.

Update: https://github.com/OrdnanceSurvey/mapbox-vector-tile-java/tree/feature/layer-enhancement

Update2: added encoder / decoder util, which could be enhanced to be dev friendly.

ShibaBandit commented 6 years ago

See suggested edits at https://github.com/OrdnanceSurvey/mapbox-vector-tile-java/pull/1

snodnipper commented 6 years ago

PR has latest code (squashed). I am remote today (limited battery / network) so if you'd like to keep more history then I'll have to deal with that tomorrow.

ShibaBandit commented 6 years ago

Merged, thank you for your contributions. Will look into maven central release jar very soon with new 2.0.0 version.

ShibaBandit commented 6 years ago

Available in 2.0.0 in maven central. @snodnipper