Open snodnipper opened 6 years ago
Regarding JTS equality, equals() contract: Very good observations/points made on that. Thank you for taking the time to do the write-up and link to the JTS issue. What you've pointed out would be a bug since it doesn't obey the equals() contract.
I would like to keep the library easy to use as possible, so I'm not immediately in favor of proposed code like in JtsLayer#checkAttributes(). I think using Integers/Floats values is handled by MvtValue + VectorTile.Tile.Value.Builder - it should end up as a long.
I don't know if many applications would need/benefit from a equals() overload vs doing their own comparisons in a static method. Also alternative to changing the API could be using this - https://commons.apache.org/proper/commons-collections/apidocs/org/apache/commons/collections4/CollectionUtils.html#isEqualCollection(java.util.Collection,%20java.util.Collection)
Regarding the role of JTS classes: I liked JtsLayer since it wasn't a heavy object but solved the problem of associating a Layer name and its contents in a simple way. It might actually be easier to remove the immutable comment as a 'documentation bug', because adding in unmodifiable at this point would actually be more of a breaking change.
If we want to make breaking changes again... JtsLayer could become an interface with mutable and immutable implementations. 'DefaultJtsLayer' (dumb bean-style) and 'ImmutableJtsLayer' (immutable and defensive copies).
I'm also watching the spec development for the next version of Vector Tiles which may be a good time to make breaking API changes again.
Sweet - I am using the library to test out some analysis routines so it has been good to kick the tyres with the lib. I'll feedback if I encounter other things.
I can certainly raise the JTS issue upstream, although talking informally with folks here there were mixed opinions about whether mixing geometry and feature concerns was a good thing for JTS (e.g. not use getUserData() in JTS for attributes). Their reasoning was that JTS is focused on the geometry concern, a subset of a GIS feature. Regardless, the JTS equality check still fails the equals contract imo.
re int/long - I encountered that attributes issue when comparing in memory geometry with int attributes against decoded long values...where the geom equality checks failed. Easily done if, say, we add age -> 45
rather than 45L
.
I can force the values to be longs although I might explore something like android's ContentValues for, say, an MvtFeature as an experiment. That is, put(String key, int value) -> is stored immediately as a long to satisfy the equality contract.
+1 on CollectionUtils - that would certainly work. We'd probably, however, still encounter the hashcode differences. This is largely an issue for folks using MVTs and Java collections - which might not be too common for the moment!
Enjoy the weekend.
Equality is failing because JTS does not consider user objects (attributes).
My suggestion is that we create an MvtFeature class that encapsulates both the geometry and the attributes (e.g. forcing underlying long values rather than allowing ints). We can then compare the JTS geometries and attributes independently. Such a class would also allow us to remove the committed but dirty code from the JtsLayer.
Furthermore, it would be good to consider the role of the JTS classes. Specifically, we marked .getGeometries as read-only. I committed code with
Collections.unmodifiableList(...)
whilst working on the equality aspects. Clearly we could keep a mutable list and add, say, anaddFeature
method...but then we'd lose the immutable benefits (final/unmodifiable).I wanted to get some visibility of this early before too much effort goes into it.
Note: there is an open issue on equality with JTS.
Draft code fixes: https://github.com/OrdnanceSurvey/mapbox-vector-tile-java/tree/bug/missingattributes