Allow GeoJSONWriter to force polygons to be CCW #694

ruibritopt · 2021-02-15T13:23:32Z

A best-effort PR to enforce the right hand rule for polygon ring orientation in the latest GeoJson specification and discussed in issue #681

Signed-off-by: Rui Brito ruibritopt@gmail.com

…s fails Signed-off-by: Rui Brito <ruibritopt@gmail.com>

Signed-off-by: Rui Brito <ruibritopt@gmail.com>

dr-jts · 2021-02-16T19:56:24Z

I would prefer to not bake this into the GeoJSONWriter. Instead could create a ForceRHR function to process each geometry beforehand?

ruibritopt · 2021-02-16T20:10:35Z

@dr-jts That is a valid point. If I understand your comment correctly, we would end up with a function like public static Geometry forceHRH(Geometry geometry) { ... }.
Would this function have to be called by the library clients before invoking write or would we extend the write function as well? Like eg: public String write(Geometry geometry, Boolean forceRHR) { ... }

dr-jts · 2021-02-17T05:01:41Z

Actually it will be more efficient if the orientation is modified on a per-ring basis in the writer. In this case, it would be best to not change the current behaviour. So an option can be added to the GeoJSONWriter (called say setForceRHR) which will check and reorient polygon rings as required. The option can be disabled by default.

…e logic Signed-off-by: Rui Brito <ruibritopt@gmail.com>

…ransformation functions Signed-off-by: Rui Brito <ruibritopt@gmail.com>

…rays so I switched to use list Signed-off-by: Rui Brito <ruibritopt@gmail.com>

ruibritopt · 2021-02-17T13:34:51Z

@dr-jts I extended the GeoJsonWriter class to have an attribute isRHREnforced, very similar to how the isEncodeCRS is set up. If I understood you correctly this was your recommendation.
The reason why the transformation function are outside the GeoJsonWriter class is because I believe the same logic could be used on the GeoJsonReader side, for the clients that would like to store geometries under the specification.

Please let me know what you think. I'm willing to make any changes necessary. Also, if the idea of implementing the same logic on the reader side is appealing, let me know and I will make another PR. Thank you

dr-jts · 2021-02-17T17:34:35Z

modules/io/common/src/main/java/org/locationtech/jts/io/geojson/TransformationUtils.java

+
+        final LinearRing rightHandRuleRing;
+        if (isExteriorRing) {
+            rightHandRuleRing = isRingClockWise? ring.reverse() : ring;


need to copy the ring if it is unchanged, to avoid aliasing errors

I believe the change I made resolves this concern.

dr-jts · 2021-02-17T17:34:46Z

modules/io/common/src/main/java/org/locationtech/jts/io/geojson/TransformationUtils.java

+        if (isExteriorRing) {
+            rightHandRuleRing = isRingClockWise? ring.reverse() : ring;
+        } else {
+            rightHandRuleRing = isRingClockWise? ring : ring.reverse();


copy here too

dr-jts · 2021-02-17T17:36:01Z

modules/io/common/src/main/java/org/locationtech/jts/io/geojson/TransformationUtils.java

+import java.util.ArrayList;
+import java.util.List;
+
+public class TransformationUtils {


How about calling this OrientationTransformer?

I followed this recommendation 👍

dr-jts · 2021-02-17T17:36:35Z

modules/io/common/src/main/java/org/locationtech/jts/io/geojson/TransformationUtils.java

+     * @param geometry to be transformed
+     * @return Geometry under the Right Hand Rule specifications
+     */
+    public static Geometry enforceRightHandRuleOrientation(final Geometry geometry) {


just call this transformRightHandRule.

I followed this recommendation 👍

dr-jts · 2021-02-17T17:39:05Z

@dr-jts I extended the GeoJsonWriter class to have an attribute isRHREnforced, very similar to how the isEncodeCRS is set up. If I understood you correctly this was your recommendation.

Yes, that looks good.

The reason why the transformation function are outside the GeoJsonWriter class is because I believe the same logic could be used on the GeoJsonReader side, for the clients that would like to store geometries under the specification.

It's good to have the RHR conversion factored out. But I don't think it's necessary to bake it into GeoJSONReader. Clients can simply transform geometries if they want to after reading them.

dbaston · 2021-02-17T18:24:36Z

Maybe consider avoiding the term RHR, since it is used by other software such as PostGIS to mean the opposite of what it means here.

dr-jts · 2021-02-17T23:42:53Z

Maybe consider avoiding the term RHR, since it is used by other software such as PostGIS to mean the opposite of what it means here.

This is a good point. To be fair, the GeoJSON spec uses the term "right-hand rule", and defines that to mean "shell is CCW". That seems to be the most commonly accepted definition of RHR. The PostGIS usage is opposite, and is deprecated (discussion here). (SQL Server seems to have the same problem).

But it's good to avoid stepping into that minefield. Also, two important design criteria for an API are consistency and parsimony of terminology. The term "CCW" is already used for this concept in JTS, and it is unambiguous. So this PR should change to using that acronym. So use isEnforceCCW, setEnforceCCW, transformCCW, etc.

The GeoJSONWriter Javadoc can point out that in the GeoJSON context "right-hand rule" = CCW.

…to CCW Signed-off-by: Rui Brito <ruibritopt@gmail.com>

Signed-off-by: Rui Brito <ruibritopt@gmail.com>

ruibritopt · 2021-02-18T16:36:55Z

Thank you @dr-jts @dbaston. I've implemented your suggestions. Please let me know if it fits your views.

dr-jts · 2021-02-18T19:35:10Z

modules/io/common/src/main/java/org/locationtech/jts/io/geojson/GeoJsonWriter.java

@@ -88,6 +89,16 @@ public void setEncodeCRS(boolean isEncodeCRS) {
    this.isEncodeCRS  = isEncodeCRS;
  }

+  /**
+   * Sets whether the GeoJSON should be output following counter-clocked orientation aka Right Hand Rule defined in RFC7946


should be "counter-clockwise".

Silly mistake, thank you for spotting it.

dr-jts · 2021-02-18T19:37:02Z

modules/io/common/src/main/java/org/locationtech/jts/io/geojson/OrientationTransformer.java

+import java.util.ArrayList;
+import java.util.List;
+
+public class OrientationTransformer {


Add class Javadoc. Can be something simple like

Utilities to modify the ring orientation of polygonal geometries.

Took your recommendation verbatim

dr-jts · 2021-02-18T19:37:47Z

Looking good. Just a couple of small things to fix, and it will be good to merge.

Signed-off-by: Rui Brito <ruibritopt@gmail.com>

dr-jts · 2021-02-19T17:42:18Z

modules/io/common/src/main/java/org/locationtech/jts/io/geojson/GeoJsonWriter.java

+   *
+   * @param enforceCCW true if the GeoJSON should be output following the RFC7946 counter-clockwise orientation aka Right Hand Rule
+   */
+  public void setEnforceCCW(boolean enforceCCW) {


One final request. In the interest of brevity, can this be changed to setForceCCW and isForceCCW?

Done and done. Don't hold back on the requests, they have all been on the money and I'm a big fan of things done right 😄👍👍

Signed-off-by: Rui Brito <ruibritopt@gmail.com>

…-hand-rule' into feat/RFC-7946-geojson-spec-right-hand-rule

ruibritopt added 5 commits February 15, 2021 14:19

correct parameter order on assertEquals, avoiding confusion when test…

49284e7

…s fails Signed-off-by: Rui Brito <ruibritopt@gmail.com>

fix tests that would fail the new right hand rule

3cdc88a

Signed-off-by: Rui Brito <ruibritopt@gmail.com>

right hand rules tests for polygon with exterior and polygon with hole

0054db6

Signed-off-by: Rui Brito <ruibritopt@gmail.com>

enforce right hand rule when writing the geojson string

f6fd350

Signed-off-by: Rui Brito <ruibritopt@gmail.com>

Merge branch 'master' into feat/RFC-7946-geojson-spec-right-hand-rule

934bac8

ruibritopt added 4 commits February 17, 2021 10:54

Merge branch 'master' into feat/RFC-7946-geojson-spec-right-hand-rule

9ab1f57

creation of a TransformationUtils class that holds the right hand rul…

e4f26a1

…e logic Signed-off-by: Rui Brito <ruibritopt@gmail.com>

added isRHREnforced option to GeoJsonWriter class which invokes the t…

aa689ba

…ransformation functions Signed-off-by: Rui Brito <ruibritopt@gmail.com>

i was getting index out of bounds with an implementation that uses ar…

63b4708

…rays so I switched to use list Signed-off-by: Rui Brito <ruibritopt@gmail.com>

dr-jts reviewed Feb 17, 2021

View reviewed changes

ruibritopt added 2 commits February 18, 2021 14:15

resolved potential aliases errors && renamed right hand rule methods …

4650572

…to CCW Signed-off-by: Rui Brito <ruibritopt@gmail.com>

renamed right hand rule method args to CCW

6aa05b4

Signed-off-by: Rui Brito <ruibritopt@gmail.com>

dr-jts changed the title ~~[Enhancement] [jts-io] Enforce right hand rule when writing GeoJSON~~ Allow GeoJSONWriter to force polygons to be CCW Feb 18, 2021

dr-jts added jts-io type-improvement labels Feb 18, 2021

dr-jts reviewed Feb 18, 2021

View reviewed changes

extended javadoc information

666a775

Signed-off-by: Rui Brito <ruibritopt@gmail.com>

Merge branch 'master' into feat/RFC-7946-geojson-spec-right-hand-rule

8bca609

dr-jts reviewed Feb 19, 2021

View reviewed changes

ruibritopt added 3 commits February 22, 2021 09:40

renamed arguments for the sake of brevity

d027011

Signed-off-by: Rui Brito <ruibritopt@gmail.com>

Merge remote-tracking branch 'origin/feat/RFC-7946-geojson-spec-right…

77300ae

…-hand-rule' into feat/RFC-7946-geojson-spec-right-hand-rule

Merge branch 'master' into feat/RFC-7946-geojson-spec-right-hand-rule

4a2e129

dr-jts merged commit 279ab3c into locationtech:master Feb 25, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow GeoJSONWriter to force polygons to be CCW #694

Allow GeoJSONWriter to force polygons to be CCW #694

ruibritopt commented Feb 15, 2021

dr-jts commented Feb 16, 2021 •

edited

Loading

ruibritopt commented Feb 16, 2021 •

edited

Loading

dr-jts commented Feb 17, 2021

ruibritopt commented Feb 17, 2021

dr-jts Feb 17, 2021

ruibritopt Feb 18, 2021

dr-jts Feb 17, 2021

dr-jts Feb 17, 2021

ruibritopt Feb 18, 2021

dr-jts Feb 17, 2021

ruibritopt Feb 18, 2021

dr-jts commented Feb 17, 2021

dbaston commented Feb 17, 2021

dr-jts commented Feb 17, 2021

ruibritopt commented Feb 18, 2021 •

edited

Loading

dr-jts Feb 18, 2021

ruibritopt Feb 19, 2021

dr-jts Feb 18, 2021

ruibritopt Feb 19, 2021

dr-jts commented Feb 18, 2021

dr-jts Feb 19, 2021

ruibritopt Feb 22, 2021

Allow GeoJSONWriter to force polygons to be CCW #694

Allow GeoJSONWriter to force polygons to be CCW #694

Conversation

ruibritopt commented Feb 15, 2021

dr-jts commented Feb 16, 2021 • edited Loading

ruibritopt commented Feb 16, 2021 • edited Loading

dr-jts commented Feb 17, 2021

ruibritopt commented Feb 17, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dr-jts commented Feb 17, 2021

dbaston commented Feb 17, 2021

dr-jts commented Feb 17, 2021

ruibritopt commented Feb 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dr-jts commented Feb 18, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dr-jts commented Feb 16, 2021 •

edited

Loading

ruibritopt commented Feb 16, 2021 •

edited

Loading

ruibritopt commented Feb 18, 2021 •

edited

Loading