TDesjardins / gwt-ol

GWT wrapper for OpenLayers 3+ using JSInterop
Apache License 2.0
70 stars 33 forks source link

MultiPoint setCoordinates #110

Closed WORMSS closed 7 years ago

WORMSS commented 7 years ago

Hello, I am new to all the OL3 stuff, so please excuse if there is something I am not understanding..

But I picked up some code from a co-worker and noticed certain things broken when trying to use setCoordinates for MultiPoint.

In my investigation something that was puzzling me was MultiPoint wanted a 2 dimensional array of Coordinates. But I was trying to understand why. I since I was new, I thought maybe it was treating MultiPoint as an array of LineStrings which only has 1 Coordinate in each string.

But this too seemed to not work when trying it.

I found here on the OL3 Documentation that it expects an Array of Coordinates (as I would have thought it would) MultiPoint#setCoordinates but MultiPoint in this project is a SimpleGeometryMultiCoordinates which expects Coordinate[][]

Can anyone explain why this is a SimpleGeometryMultiCoordinates and not a SimpleGeometryCoordinates which expects Coordinate[]

PS: to get around my problem, I made a new MultiPoint and pushed the Coordinates in 1 by 1 using appendPoint() instead.

Point pointToRemove; // From some other place.
MultiPoint origMP; // From some other place.
MultiPoint newMP = new MultiPoint();
Stream.of(origMP.getPoints())
    .filter(p -> !GeometryUtils.isGeometrySame(pointToRemove, p))
    .forEach(newMP::appendPoint);
// Set newMP in origMP's place.
WORMSS commented 7 years ago

My co-worker has rebuilt the gwt-ol3 jar for our project and changed the extension of MultiPoint from SimpleGeometryMultiCoordinates to SimpleGeometryCoordinates

Everything all seems to work with no problems and was able to go back to using the code minus the extra dimension of the array.

Point pointToRemove; // From some other place.
MultiPoint mP; // From some other place.
Coordinate[] points = Stream.of(mP.getPoints())
    .filter(p -> !GeometryUtils.isGeometrySame(pointToRemove, p))
    .map(Point::.getCoordinates)
    .toArray(Coordinate[]::new);
mP.setCoordinates(points, null); 
/* null is now required as there is no setCoordinates that only accepts the coordinates like SimpleGeometryMultiCoordinates could previously. */
TDesjardins commented 7 years ago

@WORMSS Yes, seems that inheriting from SimpleGeometryCoordinates is the correct solution. @sebasbaumh Any objection?

sebasbaumh commented 7 years ago

@WORMSS and @TDesjardins : Yes, you are right. SimpleGeometryCoordinates is the correct base class. The getCoordinates function in MultiPoint shows the correct Array.<ol.Coordinate> in contrast to getCoordinates function in MultiLineString, which returns a Array.<Array.<ol.Coordinate>>.

So feel free to change the base class to the correct one.

TDesjardins commented 7 years ago

@sebasbaumh :thumbsup: Thanks! I will fix this. By the way I noticed that method setCoordinates of ol.geom.MultiPolygon has a 3 dimensional array (see http://openlayers.org/en/master/apidoc/ol.geom.MultiPolygon.html#setCoordinates). So inheriting from SimpleGeometryMultiCoordinates may not be sufficient.

WORMSS commented 7 years ago

@TDesjardins @sebasbaumh would the second setCoordinates(Coordinates[]) be added to SimpleGeometryCoordinates ??

SimpleGeometryMultiCoordinates and SimpleGeometryMultiMultiCoordinates both have 2 setCoordinates functions without the nullable layout object.

sebasbaumh commented 7 years ago

Yes, and it also returns a 3 dimensional array - so we might have to check all of the geometry classes. On the first look it seems it is the only one working with such coordinates.

Basically I introduced SimpleGeometryCoordinates and SimpleGeometryMultiCoordinates to provide and share the correct getters and setters in different types of geometries. So maybe we should just treat MultiPolygon separate (not deriving from SimpleGeometryMultiCoordinates) and change its getter/setter for coordinates to a 3 dimensional array.

sebasbaumh commented 7 years ago

@WORMSS: Yes, that could be done as it is the same as passing null for geometryLayout to the existing setCoordinates. So feel free to add the function without the second parameter.

WORMSS commented 7 years ago

@sebasbaumh I was just talking to a co-worker, and it seems we also did a similar fix for MultiPolygon to our local copy as well, (I was unaware of this). We have SimpleGeometryMultiMultiCoordinates which actually takes a object that pretends to be a Coordinate[][][].. If I had known, I would have mentioned this also. I just presumed it was standard gwt-ol3

sebasbaumh commented 7 years ago

@WORMSS No problem :-) You might not even need a new SimpleGeometryMultiMultiCoordinates class as MultiPolygon is the only class using a 3 dimensional array.

I introduced the SimpleGeometry*Coordinates classes only to have Point inherit the SimpleGeometry class though it only has a single coordinate. So if MultiPolygon is still a SimpleGeometry adding the methods directly to it would be enough.

TDesjardins commented 7 years ago

@WORMSS I changed the code as discussed. Could you test this?

WORMSS commented 7 years ago

Yeah.. I will have to unpick what we have for MutliPolygon My co-worker says we were having problems with 3 dimensional arrays with GWT so we came up with Coordinate3D

public class Coordinate3D extends JavaScriptObject {
    protected Coordinate3D() {}
    public final native int getLength() /*-{
        return this.length;
    }-*/;
    public final native Coordinate[][] get(int i) /*-{
        return this[i];
    }-*/;
    public final native void add(Coordinate[][] coords) /*-{
        this.push(coords);
    }-*/;
}

Give me a second to see of Coordinate[][][] works.