[
https://issues.apache.org/jira/browse/LUCENE-7157?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15222731#comment-15222731
]
Robert Muir commented on LUCENE-7157:
-------------------------------------
{quote}
Just to clarify: the class allows access to the polygon information directly,
without going through methods that compute things, so I'm good with using it as
is. If those accessor methods weren't there I wouldn't be good with it. But
that is, as you say, a detail, and it can be worked out in the future.
{quote}
The intent of this class in my eyes, was just to be a holder class only. E.g.
something solely for users so we don't have apis taking double[][] or anything
like that from the user. As simple as possible without e.g.
Polygon/MultiPolygon/LineString/Ring/Hole/Builder/Factory etc. So the idea was
a {{XYZPoint.newPolygonQuery(String field, Polygon...)}}. This makes simple
things simple and complex things possible with minimal API surface area.
I do think it makes sense for it to have checks up-front so that illegal
polygons are not possible to create. I also think this should map towards
standard representations (like GeoJSON and WKT and so on) and it does. Ideally
its as minimal as that.
However, it currently serves extra duties: its not just dealing with user
input, but also being "held" by queries for e.g. equals and hashcode. This also
means it should be strictly immutable to avoid traps with query-caching. And we
have to be careful since we are using arrays. But maybe this is a duty it
should really have? Because we've had a considerable number of equals/hashcode
bugs in Points so far!
Finally it contains the 2D logic (contains/intersects/etc) that we have today
simply because that was the safest, easiest to test way to cutover the two
different 2D geo impls (GeoPoint and LatLonPoint) over with support for holes
and multiple rings. Previously these were static methods taking e.g. 8-10
parameters and it was hard to enforce basics things (like bounding box checks
always used before expensive polygon methods).
I hope these 2D methods don't stay here too, mainly because I think we can make
better-performing impls that scale better for complex polygons (LUCENE-7159).
So i'd hope we'd move away from those methods and then they would go away as
part of that issue. But we cannot do such things until we have better edge case
testing. Historically testing was done with epsilons and i am battling a ton of
edge case bugs in even stuff like basic bounding box queries.
Maybe in the meantime we could try to move them to another place in the same
package instead. With this representation, things cannot arbitrarily nest, so
maybe they could just be static methods that use nested loops?
{quote}
I'm trying quite hard here to follow your lead on API since you seem to be the
person who is setting that. I was suddenly handed a huge concern about how the
Geo3DPoint API's looked and an utterly non-negotiable requirement that they
look identical to the 2D APIs, and I've been scrambling to keep up ever since.
Mike's proposal to fork the Polygon class was his attempt to be helpful here,
but I'm not going forward anyhow until all required API classes are publicly
available within spatial3d, however that is done.
{quote}
I think Nick's issue LUCENE-7163 will fix your problem there. As far as
"utterly non-negotiable requirement that 3D look identical to 2D apis", I don't
see it that way. Instead I'll ask, why wouldn't we offer a basic api with just
one class that covers the major operations you can do? We can always offer
expert apis some other way.
To me what geo3d does is implementation detail: the user shouldn't even need to
know that 3d math is being used.
Just like I don't think users need to deal with radians, I don't think they
should know about concaveness either. I understand that geo3d is different, but
it needs to not unload the challenges of 3d-ness onto the user but instead use
it behind the scenes.
> Geo3DPoint implementation should pay attention to the ordering of lat/lon
> points
> --------------------------------------------------------------------------------
>
> Key: LUCENE-7157
> URL: https://issues.apache.org/jira/browse/LUCENE-7157
> Project: Lucene - Core
> Issue Type: Bug
> Reporter: Karl Wright
>
> The LatLonPoint API implementation pays attention to the order of the points;
> "clockwise" means one side of the polygon boundary, and "counterclockwise"
> means the complement. We need to use that in Geo3DPoint and convert into
> whatever the underlying GeoPolygonFactory method requires.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
