[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

[task #13658] Work on concave polygons too

From: Mohammad Akhlaghi
Subject: [task #13658] Work on concave polygons too
Date: Wed, 8 Apr 2020 10:01:40 -0400 (EDT)
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:74.0) Gecko/20100101 Firefox/74.0

Follow-up Comment #44, task #13658 (project gnuastro):

Great! Thanks Sachin. A few small points remain, I look forward to merging it

* The commit isn't on the most recent commit of the master branch. You can
fetch the most recent commit(s) on the master branch and then rebase over

* The static structures and functions that are only defined in `polygon.c'
(and not `polygon.h') are only relevant during the building of the library.
They aren't accessible for a user of Gnuastro once it is installed. So they
don't need documentation in the book. They should only be documented in the
actual source as comments.

* In the documentation of `gal_polygon_vertices_sort', it would be really
helpful if you explain the methodology of your algorithm a little. In
particular note that for concave polygons there is no unique sorting and that
people should beware (possibly using the polygon-type function to make sure if
it is a convex or concave polygon afterwards). Also, do this when you call
this function in the Crop program's `onecrop.c': put a check after the sorting
and if the polygon is convex, print a warning, with `error(0,0,"WARNING:
...."')  and let the user know that the sorted convex polygon may not be what
they wanted.

* You can set `struct point' to be static in `polygon.c'.

* I noticed multiple return statements in `polygon_compareA' and
`polygon_compareB' that can be made more readable with the `?:' structure

* Finally, two extra curly-brace blocks (after the `for's) can be removed in
the final set of loops in `gal_polygon_vertices_sort'. Extra curly braces can
be buggy and will always confuse the reader on why you used them? For example
was there something you wanted to change but forgot?


Reply to this item at:


  Message sent via Savannah

reply via email to

[Prev in Thread] Current Thread [Next in Thread]