Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ST_3DDistance: Handle invalid polygons #338

Closed
wants to merge 5 commits into from

Conversation

@Algunenano
Copy link
Member

commented Nov 20, 2018

  • Added guards in "measures.h" since it didn't have them.
  • Simplified define_plane to calculate the plane with the first 3 non collinear points of the ptarray.
  • Added tests in liblwgeom as it was easier to debug.
  • Avoid division by zero in lw_dist3d_ptarray_poly when the point is the same plane as the polygon.
  • Add a bunch of extra tests (some copied from the 2d tests, some extras around the same plane issue).
  • When a 3d polygon is defined by collinear points, consider it as a 3d linestring (by ignoring any inner ring).

Trac issue: https://trac.osgeo.org/postgis/ticket/4246

@Komzpa
Komzpa approved these changes Nov 20, 2018
return 0.0;

f = DOT(pl->pv, v1);
if (fabs(f) < DBL_EPSILON)

This comment has been minimized.

Copy link
@Komzpa

Komzpa Nov 20, 2018

Member

didn't we have FP_IS_ZERO for such things?

@@ -58,7 +59,7 @@ get_3dcross_product(VECTOR3D *v1,VECTOR3D *v2, VECTOR3D *v)
v->y=(v1->z*v2->x)-(v1->x*v2->z);
v->z=(v1->x*v2->y)-(v1->y*v2->x);

return LW_TRUE;
return ((fabs(v->x) > DBL_EPSILON) || (fabs(v->y) > DBL_EPSILON) || (fabs(v->z) > DBL_EPSILON));

This comment has been minimized.

This comment has been minimized.

Copy link
@Algunenano

Algunenano Nov 20, 2018

Author Member

👍 I didn't remember about that macro

@@ -1133,63 +1164,46 @@ the plane is stored as a point in plane (plane.pop) and a normal vector (plane.p
int
define_plane(POINTARRAY *pa, PLANE3D *pl)

This comment has been minimized.

Copy link
@nicklasaven

nicklasaven Nov 20, 2018

Contributor

I am not sure about the simplification of define_plane. The reason I did it this complicated was an attempt to avoid small numbers giving large errors. If the first 3 points is very close to each other a very small error (floating point error) can make the plane quite wrong which can give quite big error if the polygon is large. This is far away in my head no, since it was a long time ago. I will look more into it.

This comment has been minimized.

Copy link
@Algunenano

Algunenano Nov 20, 2018

Author Member

Maybe we could try to add some test with really close numbers (but bigger than FP_TOLERANCE) to see if it reproduces it.

This comment has been minimized.

Copy link
@nicklasaven

nicklasaven Nov 20, 2018

Contributor

Yes, I think we need to think about stability for cases like that and also "dirty" polygons, that has more or less coplanar vertex points.

This comment has been minimized.

Copy link
@Algunenano

Algunenano Nov 20, 2018

Author Member

BTW, since now when we can't calculate the plane it defaults to checking the external ring itself as a line, we could add an extra check in define_plane and ignore any point whose distance to the previous ones is less than FP_TOLERANCE or something like this.

@nicklasaven

This comment has been minimized.

Copy link
Contributor

commented Nov 21, 2018

Here is the thoughts behind how the define_plane function is designed now.

https://trac.osgeo.org/postgis/wiki/3DDistancecalc

@Algunenano

This comment has been minimized.

Copy link
Member Author

commented Nov 21, 2018

I've modified my define_plane to calculate the average of 3 normals if possible. @nicklasaven Does it makes sense?

@nicklasaven

This comment has been minimized.

Copy link
Contributor

commented Nov 22, 2018

Yes, I think it looks good. If I understand it right the functionality is the same as before, but you use a fixed 3 samples instead of my walabout ending with 3-5 samples. Is that correct?
If line 1197 - 1202 is safe from any pitfalls I think it looks very good. Much cleaner than before.
I would have put some floor in the above mentioned rows just because I am not that confident about the integer divisions. But if this is safe it is great.

@Algunenano

This comment has been minimized.

Copy link
Member Author

commented Nov 22, 2018

Looking into it again I see that I'm using npoints instead of (npoints - 1) so I'm repeating the first point (equal to last). I'll change that before merging.

Int division always truncates. Floor would just cast it to float and back, which hopefully will be optimized away by the compiler and do nothing ;D

@strk strk closed this in 03ed7d3 Nov 22, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.