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

NEw ST_Angle function, with test and doc #97

Closed
wants to merge 15 commits into
base: svn-trunk
from

Conversation

Projects
None yet
3 participants
@Remi-C

Remi-C commented Feb 25, 2016

Hey,
new function with 2 behaviours,
ST_Angle(P1,P2,P3) returns the angle of P1P2P3
ST_ANgle(P1,P2,P3,P4) returns the angle of 2 vectors P1P2,P3P4

For an easy visual test, use the query in the doc to generate a few dozen angles, and display in qgis.
Cheers

n_args = i ;
if (n_args < 3)
lwpgerror("Empty geometry");
break;

This comment has been minimized.

@strk

strk Feb 25, 2016

Member

Should this be a PG_RETURN_NULL() instead ?

This comment has been minimized.

@strk

strk Feb 25, 2016

Member

Sorry, I meant within the conditional block, not outside

<refnamediv>
<refname>ST_Angle</refname>
<refpurpose>Returns the angle in radian measured clokwise between 3 points, or the angle measured clockwise between 2 vectors.</refpurpose>

This comment has been minimized.

@strk

strk Feb 25, 2016

Member

"clokwise" -> "clockwise" (first occurrence)

If I may suggest also keeping the "refpurpose" text small (may omit radian and clockwise, leaving it specified in the "description")

<paramdef><type>geometry </type><parameter>point1</parameter></paramdef>
<paramdef><type>geometry </type><parameter>point2</parameter></paramdef>
<paramdef><type>geometry </type><parameter>point3</parameter></paramdef>
<paramdef><type>geometry </type><parameter>point4</parameter></paramdef>

This comment has been minimized.

@strk

strk Feb 25, 2016

Member

Isn't the 4th point optional ? There should be a tag or attribute to specify that in the docs.

<para> For 3 points, computes the angle measured clockwise of P1P2P3.
For 4 points,compute the angle measured clockwise of P1P2,P3P4.
Results are always positive, between 0 and 2*Pi radians.
Uses ST_Azimuth.

This comment has been minimized.

@strk

strk Feb 25, 2016

Member

"Uses ST_Azimuth" isn't really true, it's using liblwgeom internal functions (on which ST_Azimuth is also based).

lwpoint = lwgeom_as_lwpoint(geom_unser);
if ( ! lwpoint )
{
PG_FREE_IF_COPY(geom, i);

This comment has been minimized.

@strk

strk Feb 25, 2016

Member

These memory releases should really affect all the previous geometry inputs, or only the last one would be released.

if ( ! getPoint2d_p(lwpoint->point, 0, &points[i]) )
{
PG_FREE_IF_COPY(geom, i);
lwpgerror("Error extracting point");

This comment has been minimized.

@strk

strk Feb 25, 2016

Member

This should really only happen when the point is empty (hint hint [ for reducing complexity ])

if (n_args == 3)
{
if ( ! azimuth_pt_pt(&points[0], &points[1], &az1) )
PG_RETURN_NULL();

This comment has been minimized.

@strk

strk Feb 25, 2016

Member

I guess the code here expects azimuth_pt_pt to call lwerror itself, better specify it with a comment, or a reader would be wondering why no error message is printed.

An lwpgerror could even be left to be even ready shall liblwgeom stop reporting errors in such a primitive way :)

ST_Angle_4_pts|4.71238898038469
ST_Angle_4_pts|0.785398163397448
ST_Angle_3_pts|1.5707963267949
ERROR: Operation on mixed SRID geometries

This comment has been minimized.

@strk

strk Feb 25, 2016

Member

The EMPTY case is missing a test

This comment has been minimized.

@Remi-C

Remi-C Feb 25, 2016

Hey,
I removed the empty test case because ST_Azimuth is failing it.
(it returns another error than the one expected)
it returns ERROR: getPoint2d_p: point offset out of range

This comment has been minimized.

@Remi-C

Remi-C Feb 25, 2016

hm never mind its ok I added the missing test, thanks

This comment has been minimized.

@strk

strk via email Feb 25, 2016

Member
@Remi-C

This comment has been minimized.

Remi-C commented Feb 25, 2016

Hey @strk , thanks for the review!
I don't get the PG_RETURN_NULL stuff,
what do you mean exactly?
(I don't get either the error you mention, are you pointing to a line in perticular (I don't know how to see it if this is the case))
I corrected the other things you mention, now working on the memory stuff.
Cheers

@strk

This comment has been minimized.

Member

strk commented Feb 25, 2016

@Remi-C

This comment has been minimized.

Remi-C commented Feb 25, 2016

@strk I don't get the memory issue you mention.
POINT2D points[4]; doesn't need to be freed manually I guess.
Other variables are freed in each loop, so no need to free those in case of error some loop after?
Do you mean I should free geom_unser and / or lwpoint
before each PG_RETURN_NULL ?
Cheers

@strk

This comment has been minimized.

Member

strk commented Feb 25, 2016

@Remi-C

This comment has been minimized.

Remi-C commented Feb 26, 2016

Hey @strk, here is a rewrite,
with better separation of exception and actual computing.
Exception are tried on serialized geometry.
About PG_FREE_IF_COPY,
from what I understand there is no need to use it on a parameter if you havn't used PG_GETARG_GSERIALIZED_P.
If this is true, it should be ok as it is now (only one arg is in memory at a time, being cleaned at the end of the loop anyway)
Cheers,
Remi-C

@strk

This comment has been minimized.

Member

strk commented Feb 26, 2016

Ok so I see you're detoasting/free-if-copying once for checking and again for actual using.
Could you avoid the duplication ?

if (!lwpoint)
{
lwpgerror("Error unserializing geometry");
PG_RETURN_NULL() ;

This comment has been minimized.

@strk

strk Feb 26, 2016

Member

Also this misses a PG_FREE_IF_COPY...

This comment has been minimized.

@Remi-C

Remi-C Feb 26, 2016

@strk Ok as you suggested I stored the serialized pointers to keep getting
those twice.
Now PG_FREE_IF_COPY is always executed if exceptions or in regular
operations.

2016-02-26 14:47 GMT+01:00 Sandro Santilli notifications@github.com:

In postgis/lwgeom_functions_basic.c
#97 (comment):

  •       case 3:
    
  •       lwpgerror("Operation on mixed SRID geometries");
    
  •       PG_RETURN_NULL();
    
  •       break;
    
  •   }
    
  • /* extract points */
  • for(i=0; i<n_args; i++)
  • {
  •   geom = PG_GETARG_GSERIALIZED_P(i);
    
  •   geom_unser = lwgeom_from_gserialized(geom) ;
    
  •   lwpoint = lwgeom_as_lwpoint(geom_unser);
    
  •   if (!lwpoint)
    
  •   {
    
  •       lwpgerror("Error unserializing geometry");
    
  •       PG_RETURN_NULL() ;
    

Also this misses a PG_FREE_IF_COPY...


Reply to this email directly or view it on GitHub
https://github.com/postgis/postgis/pull/97/files#r54244102.

@Remi-C

This comment has been minimized.

Remi-C commented Feb 26, 2016

Hey @strk , here the pointers are stored to avoid calling twice the functions.
Cheers

/* dont do lwpoint_free(lwpoint); , this memory is needed ! */
}
for (j=0;j<n_args;j++)
PG_FREE_IF_COPY(seri_geoms[j], j);

This comment has been minimized.

@strk

strk Feb 26, 2016

Member

It is not safe to release the serialized format after deserializing, because LWGEOM can still refer to the serialized POINTLIST.

@Remi-C

This comment has been minimized.

Remi-C commented Feb 26, 2016

@strk : thanks to help me learn all of this.
Cheers,
RemiC

@robe2

This comment has been minimized.

Member

robe2 commented May 20, 2017

@Remi-C looks like this isn't ready yet as @strk had one last note doesn't look like you addressed. Also this needs to be rebased if to be committed for PostGIS 2.4.

@robe2

This comment has been minimized.

Member

robe2 commented Aug 10, 2017

@Remi-C can you rebase and I'll test and commit for PostGIS 2.4

@Remi-C

This comment has been minimized.

Remi-C commented Sep 5, 2017

@strk

This comment has been minimized.

Member

strk commented Sep 5, 2017

@robe2

This comment has been minimized.

Member

robe2 commented Sep 5, 2017

keep in mind it's too late for this to go into 2.4. Has to wait for 2.5

@Remi-C

This comment has been minimized.

Remi-C commented Sep 7, 2017

Hey @robe2, no problem, it'll be for 2.5 with any luck.
Thanks @strk, this fixed it (after some awkward manual edits)

Minor typos

@strk strk closed this in d60efbc Oct 4, 2017

pramsey pushed a commit to pramsey/postgis-gh that referenced this pull request Oct 16, 2017

ST_Angle function from Rémi Cura
Closes postgis/postgis#97
Closes #3876

git-svn-id: https://svn.osgeo.org/postgis/trunk@15884 b70326c6-7e19-0410-871a-916f4a2858ee

@pramsey pramsey referenced this pull request Oct 16, 2017

Closed

ST_Angle function #4072

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