Consistent circle transform. #2696

gberaudo · 2014-09-09T13:10:01Z

Depending on compilation flags, applying a transform used to either:

work;
fail throwing an abstract method not implemented message;
fail silently.

Now it should consistently work, like the other geometries.

gberaudo · 2014-09-10T08:31:26Z

Please comment so that we can move on.

ahocevar · 2014-09-10T16:13:19Z

It is indeed strange that a subclass overrides an implemented method of a superclass with an abstract method. So I tend to think that this change makes sense. However, since there is a unit test that checks for the exception being thrown because of the abstract method, there must be a reason. Maybe the same transform that's used on the superclass does the wrong thing for the circle?

gberaudo · 2014-09-12T09:13:19Z

I am using it and it is working correctly.
It seems it was (wrongly) considered absurd to project a circle.

ahocevar · 2014-09-15T11:41:10Z

Looking at http://openlayers.org/en/master/examples/tissot.html, I would think that when you want to transform a circle, you would use `ol.geom.Polygon.circular´ to create a circle - and it is indeed absurd to project a circle that is only defined by radius and center.

elemoine · 2014-09-15T13:34:30Z

It is not really about "projecting a circle". It is about transforming/re-projecting a circle (which already includes projected coordinates).

Internally, for a circle, we store its center, and the point to the right of the center. So if the center is [cx, cy] and the radius is r, we store cx, cy, cx + r, cy in the circle's flatCoordinates array. Using the point to the right of the center is arbitrary. Really we could use any point at a distance r from the center. It would equivalent…

… until we want to transform/re-project the circle. For example, a circle whose center is [0, 0] and radius is 10 degrees in EPSG:4326 is transformed to a circle whose radius is ~1113194.90 if the point to right (or left) of the center is used, and whose radius is ~1118889.97 in the point to top/bottom of the center is used.

What this means is that re-projecting a circle does not result in a circle. In the case of ol.geom.Circle we do not want transform to change the type of the geometry.

So two options: (1) we do not permit transforming a circle, or (2) we're lenient and just transform the flatCoordinates as done in @gberaudo's patch. I don't know what's the best option. What do you think?

ahocevar · 2014-09-15T13:37:28Z

I think it should be sufficient to document properly that transforming a circle geometry might give the user unexpected results (i.e. a circle), and that ol.geom.Polygon.circular should be used when circles need to be properly transformed. So @gberaudo's patch should be fine, with only a bit of documentation to add.

elemoine · 2014-09-15T13:46:29Z

I think it should be sufficient to document properly that transforming a circle geometry might give the user unexpected results (i.e. a circle), and that ol.geom.Polygon.circular should be used when circles need to be properly transformed. So @gberaudo's patch should be fine, with only a bit of documentation to add.

Agree.

Regarding ol.geom.Polygon.circular, can it be used with any coordinate system? I know it's a somewhat different subject, but I'm wondering.

gberaudo · 2014-09-15T14:54:29Z

@ahocevar , I updated the documentation, adding the following text:
Use {ol.geom.Polygon.circular} if you do not want the projection to stay a circle.

I also added a @see reference to {ol.geom.Polygon.circular} in constructor.

elemoine · 2014-09-15T15:03:35Z

src/ol/geom/circle.js

@@ -18,6 +19,7 @@ goog.require('ol.geom.flat.deflate');
 * @param {number=} opt_radius Radius.
 * @param {ol.geom.GeometryLayout=} opt_layout Layout.
 * @api
+ * @see {ol.geom.Polygon.circular}


Sorry but I think that a link with no context doesn't provide much value here. I'd rather not add it here.

gberaudo · 2014-09-15T15:55:47Z

Thank you for all comments, I hope it is ready for merging now.

elemoine · 2014-09-15T16:08:54Z

src/ol/geom/circle.js

+ * `[cx + r, cy]`. This `transform` function just transforms these two points.
+ * So the resulting geometry is also a circle, and that circle does not
+ * correspond to the shape that would be obtained by transforming every point
+ * of the original circle. It is the same for the other geometries.


This is the docs for circle. Why are you talking about "other geometries" here?

The way we formutate this we really give the impression that Circle is
special and obey special rules.

I want to clarify this point, maybe there is a better formulation?

The way we formutate this we really give the impression that Circle is special and obey special rules.

Which is true. If you reproject every point of a point, line or polygon the transformed geometry is still a point, line, polygon, respectively. If you reproject every point of a polygon whose shape is "circular" the shape of the transformed may not be circular.

You are confusing Circle and Polygon.

A 'circular' polygon is not a circle.

The reprojection of an ol.geom.Polygon, be it circular or not, is a polygon.
The reprojection of an ol.geom.Circle is a circle.

The reprojection of an ol.geom.Circle is a circle.

Depends on whether you are talking about a circle on a sphere or a circle geometry. But I don't think this discussion takes us any further. I think we have agreed already that this pull request is fine, and we can improve documentation later if we see that users run into issues.

elemoine · 2014-09-15T16:17:01Z

Sorry I still have comments. Although I agree that documentation-related review comments are often subjective. Please address my comments if you agree with them. Squashing the commits into one might make sense as well. (I'm not a big fan of "Address @elemoine comments" type commits.) Once done I'll merge. Thanks!

elemoine · 2014-09-16T09:42:39Z

src/ol/geom/circle.js

+ * then use this function on the clone.
+ *
+ * Internally a circle is currently represented by two points: the center of
+ * the circle `[cx, cy]`, and the point to the left of the circle


left -> right

Depending on compilation flags, applying a transform used to either: - work; - fail throwing an abstract method not implemented message; - fail silently. Now it should consistently work, like the other geometries. Adding a polygon factory method creating an approximation of a circle on a plane would be useful for users wanting the circle to be deformed. It would be similar to the `circular` function which creates an approximation of a circle on a sphere.

Consistent circle transform.

gberaudo mentioned this pull request Sep 10, 2014

Vector layer conversion openlayers/ol-cesium#33

Merged

gberaudo force-pushed the consistent_circle_transform branch from 81f29bd to c24deb6 Compare September 15, 2014 14:50

elemoine reviewed Sep 15, 2014
View reviewed changes

gberaudo force-pushed the consistent_circle_transform branch 2 times, most recently from 3104c99 to 8e57a36 Compare September 15, 2014 15:54

gberaudo force-pushed the consistent_circle_transform branch from 8e57a36 to 5e34606 Compare September 15, 2014 16:00

elemoine reviewed Sep 15, 2014
View reviewed changes

elemoine reviewed Sep 16, 2014
View reviewed changes

gberaudo force-pushed the consistent_circle_transform branch 2 times, most recently from d9ba151 to 9663c43 Compare September 16, 2014 11:35

gberaudo force-pushed the consistent_circle_transform branch from 9663c43 to d67cefd Compare September 16, 2014 11:41

elemoine pushed a commit that referenced this pull request Sep 16, 2014

Merge pull request #2696 from gberaudo/consistent_circle_transform

163cc5b

Consistent circle transform.

elemoine merged commit 163cc5b into openlayers:master Sep 16, 2014

elemoine mentioned this pull request Sep 16, 2014

goog.abstractMethod removed in compiled mode #2680

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Consistent circle transform. #2696

Consistent circle transform. #2696

gberaudo commented Sep 9, 2014

gberaudo commented Sep 10, 2014

ahocevar commented Sep 10, 2014

gberaudo commented Sep 12, 2014

ahocevar commented Sep 15, 2014

elemoine commented Sep 15, 2014

ahocevar commented Sep 15, 2014

elemoine commented Sep 15, 2014

gberaudo commented Sep 15, 2014

elemoine Sep 15, 2014

gberaudo commented Sep 15, 2014

elemoine Sep 15, 2014

gberaudo Sep 15, 2014

elemoine Sep 15, 2014

gberaudo Sep 16, 2014

ahocevar Sep 16, 2014

elemoine commented Sep 15, 2014

elemoine Sep 16, 2014

Consistent circle transform. #2696

Consistent circle transform. #2696

Conversation

gberaudo commented Sep 9, 2014

gberaudo commented Sep 10, 2014

ahocevar commented Sep 10, 2014

gberaudo commented Sep 12, 2014

ahocevar commented Sep 15, 2014

elemoine commented Sep 15, 2014

ahocevar commented Sep 15, 2014

elemoine commented Sep 15, 2014

gberaudo commented Sep 15, 2014

elemoine Sep 15, 2014

Choose a reason for hiding this comment

gberaudo commented Sep 15, 2014

elemoine Sep 15, 2014

Choose a reason for hiding this comment

gberaudo Sep 15, 2014

Choose a reason for hiding this comment

elemoine Sep 15, 2014

Choose a reason for hiding this comment

gberaudo Sep 16, 2014

Choose a reason for hiding this comment

ahocevar Sep 16, 2014

Choose a reason for hiding this comment

elemoine commented Sep 15, 2014

elemoine Sep 16, 2014

Choose a reason for hiding this comment