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

Fix segmentize on geography #90

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
4 participants
@mhugo

mhugo commented Feb 18, 2016

Segments were split on the straight line going from one vertex to the other and then "projected back" on the surface of the sphere. It resulted in segments with wrong distances.
Segments need to be computed based on equal angles between two adjacent vertices.

Show outdated Hide outdated regress/geography_expected
@@ -27,3 +27,4 @@ geog_precision_pazafir|0|0
#2422|1|1609|t|t|1400.230|1396.816|1400.230|1400.230
#2422|1|1600|t|t|1400.230|1396.816|1400.230|1400.230
#2422|1|1068|f|f|1400.230|1396.816|1400.230|1400.230
segmentize_geography|t

This comment has been minimized.

@strk

strk Feb 18, 2016

Member

I think it would be better to give a rounded number than a boolean value.
Otherwise a max distance of 0 would still not be caught as a failure

@strk

strk Feb 18, 2016

Member

I think it would be better to give a rounded number than a boolean value.
Otherwise a max distance of 0 would still not be caught as a failure

@mhugo

This comment has been minimized.

Show comment
Hide comment
@mhugo

mhugo Feb 18, 2016

@strk code updated with a rounded number in test

mhugo commented Feb 18, 2016

@strk code updated with a rounded number in test

@mhugo

This comment has been minimized.

Show comment
Hide comment
@mhugo

mhugo Feb 26, 2016

updated code that should now fix the broken regression test

mhugo commented Feb 26, 2016

updated code that should now fix the broken regression test

@pramsey

This comment has been minimized.

Show comment
Hide comment

@pramsey pramsey closed this Feb 26, 2016

strk pushed a commit that referenced this pull request Feb 26, 2016

@robe2 robe2 reopened this Jul 31, 2016

@robe2

This comment has been minimized.

Show comment
Hide comment
@robe2

robe2 Jul 31, 2016

Member

As noted here - https://trac.osgeo.org/postgis/ticket/3539 this change broke ST_Segmentize functionality pretty badly. Please fix or we'll have to revert.

Member

robe2 commented Jul 31, 2016

As noted here - https://trac.osgeo.org/postgis/ticket/3539 this change broke ST_Segmentize functionality pretty badly. Please fix or we'll have to revert.

@mhugo

This comment has been minimized.

Show comment
Hide comment
@mhugo

mhugo Aug 17, 2016

@robe2 @pramsey Here is the new patch that fixes both segment distances and curvatures, using sphere_direction and sphere_project functions.

mhugo commented Aug 17, 2016

@robe2 @pramsey Here is the new patch that fixes both segment distances and curvatures, using sphere_direction and sphere_project functions.

@pramsey

This comment has been minimized.

Show comment
Hide comment
@pramsey

pramsey Aug 17, 2016

Member

yeah, better to use that one. I have an approach that I think would be more efficient but I don't think I'll get it done in a short period of time...

Member

pramsey commented Aug 17, 2016

yeah, better to use that one. I have an approach that I think would be more efficient but I don't think I'll get it done in a short period of time...

@robe2

This comment has been minimized.

Show comment
Hide comment
@robe2

robe2 Aug 17, 2016

Member

Thanks. I'll test later today and then commit

Member

robe2 commented Aug 17, 2016

Thanks. I'll test later today and then commit

@robe2

This comment has been minimized.

Show comment
Hide comment
@robe2

robe2 Aug 17, 2016

Member

Test fails on sfcgal

`
regress_sfcgal .. ok
sfcgal/empty .. ok
sfcgal/geography .. ok
sfcgal/legacy .. ok
sfcgal/measures .. ok
sfcgal/regress_ogc_prep .. ok
sfcgal/regress_ogc .. ok
sfcgal/regress .. ok
sfcgal/tickets .. failed (diff expected obtained: /projects/postgis/tmp/2.3_pg9.6w64/test_126_diff)
sfcgal/concave_hull .. ok
sfcgal/wmsservers .. ok
sfcgal/approximatemedialaxis .. ok
uninstall .. ok (4578)

`

No worries, looks like you just forgot to make same change there. I'll fix on my end.

Member

robe2 commented Aug 17, 2016

Test fails on sfcgal

`
regress_sfcgal .. ok
sfcgal/empty .. ok
sfcgal/geography .. ok
sfcgal/legacy .. ok
sfcgal/measures .. ok
sfcgal/regress_ogc_prep .. ok
sfcgal/regress_ogc .. ok
sfcgal/regress .. ok
sfcgal/tickets .. failed (diff expected obtained: /projects/postgis/tmp/2.3_pg9.6w64/test_126_diff)
sfcgal/concave_hull .. ok
sfcgal/wmsservers .. ok
sfcgal/approximatemedialaxis .. ok
uninstall .. ok (4578)

`

No worries, looks like you just forgot to make same change there. I'll fix on my end.

@strk strk closed this in 598feab Aug 17, 2016

@mhugo

This comment has been minimized.

Show comment
Hide comment

mhugo commented Aug 17, 2016

@strk thanks !

@strk

This comment has been minimized.

Show comment
Hide comment
@strk

strk Aug 17, 2016

Member

Robe did all, only mirroring pushes are all in my name :)

Member

strk commented Aug 17, 2016

Robe did all, only mirroring pushes are all in my name :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment