I would like to work on this task.

This task has been assigned to Johannes Schulte. You have 72 hours to complete this task, good luck!

The work on this task is ready to be reviewed.

Johannes, this is thought-provoking but there are several issues I see.  First is that that new structure is a bit ill-defined.  It has fields that are only sometimes used but no way to know if/when they're being used.  There's no means to initialize/deinitialize one and they will have heap-allocated memory, presumably, in the pts field.

Other noteworthy comments:

• every field should have a clear purpose and be described if it's going to be a public struct, e.g., label seems to serve no purpose.


• mixing computed fields (e.g., area, cent, plane_eqn) with non-computed (e.g., cent_pyramid, pts, npts) is usually a bad idea, at least without some means to know when the computed are in-fact computed.


• unused computed fields waste space if you never need/want them.


• your notion of associating a polyhedron face based on some 3D point (cent_pyramid) is actually rather interesting, novel.


• shorter is better when there's not a competing concept, "struct bn_polygon" would be shorter and unambiguous.  note the existing bn_poly_t for polynomials, confusing.


• your structure heavily overlaps with bn_vlist and ged_poly_contour/ged_polygon/ged_polygons in the libged library so perhaps there's some way to integrate with those concepts.

Since this is technically introducing four new API concepts, I suggest you trim this way back to just one of the four concepts instead of trying to tackle so much at once.  Probably either just new structure(s) for dealing with polygons or just one of the "analysis" functions with a different more general data parameter (like just vertices).

For example, to calculate a polygon surface area, I really just need a list of vertices.  So I could describe a function that needs no structures, just taking an array of pnts and calculating the area.  Basically, a generalized version of bn_area_of_triangle().

One of the mentors has sent this task back for more work. Talk to the mentor(s) assigned to this task to satisfy the requirements needed to complete this task, submit your work again and mark the task as complete once you re-submit your work.

Thanks for your input. I lost a bit of overview. After looking again through the current usages of the poly_face, I don't think anymore, that defining this struct in a public API makes much sense. The use cases just differ too much, which ended in this overloaded struct. E.g. the label is just used by libged, which on the other hand doesn't need the _pyramid variables at all. So I will leave it up to the specific caller to define its own struct, if necessary. Nevertheless the polygon routines are needed. I hope this definitions are more general.

The work on this task is ready to be reviewed.

In fact, at least the second function header is still wrong. I missed the fourth parameter, a pointer to an array of size_t's, in which the number of points, added to the corresponding face, will be stored.

One of the mentors has sent this task back for more work. Talk to the mentor(s) assigned to this task to satisfy the requirements needed to complete this task, submit your work again and mark the task as complete once you re-submit your work.

This task is a lot harder than it seems.  It takes a lot of forethought to work through everything that should be considered when creating public API that others will use (which is the case for these public libraries -- they're used by dozens of other application codes outside BRL-CAD).  That's why I suggest limiting yourself to just one function for simplicity, I suggest it be area or centroid or both if you like, but leaving out the third function at least.

Now for the function or two that remains, a few issues of note:

• if a parameter doesn't changed by the function, it should be marked const


• don't waste a return type, suggest either returning the value since it is a single value or make it a success/failure code if there is any potential for failure (e.g., what happens when someone passes a null pointer?).


• when it comes to arrays and sizes, think of established convention: "int ac, char *av[]".  the count should come first.


• that said, modern code dictates using size_t when something is a size (so that bit is good already)


• surf_area raises questions.  If i have a polygon, what is the surface?  both sides?  one side?  The ambiquity is removed if you just say bn_polygon_area() and your comment documentation should say it's the interior area (of either side).


• the doxygen comments should be expaneded to describe all the arguments, any return values, any assumptions, what happens if NULL is passed for any pointers, etc.


• the @brief comment you have is orphaned.  each /** */ can have a @brief, or just separate a brief description like you currently have onto one line and then the details in a separate paragraph.

The work on this task is ready to be reviewed.

So, what I'm the least sure about at the moment is the assumption, that the pts array is ccw-sorted. Can I expect that from the caller, or should I do this inside the functions. The problem is, that in many cases, both functions are called almost successive, so it would it be as expensive as useless to sort the array two times. Maybe even another public function for this makes sense, so that not everytime,  someone wants to calcualte the area of a polygon, he has to work out the logic to sort it ccw (I know of at least 4 places, where this function would be used).

If you have an assumption like that, it must be documented in the API header.  Another assumption to consider is whether an end point must be provided, must NOT be provided, or does it not matter?  What about if there are other repeat points?  What if it self-intersects?  If a particular algorithm is going to be used, that would be something to include in the detailed portion too.

As for sorting... I'm not sure how you could *correctly* sort a polygon based on the user's array of points.  You could certainly do a sorting, but not necessarily the correct sorting that will result in the correct area.  You could always sort and then document that action in the header so the caller knows, or you could say it's up to the caller to provide it sorted.  Either way, it should be stated.

The general form of a header comment is:

/**
 * This is the brief.
 *
 * This covers any additional information, explanation, details,
 * caveats, assumptions, etc.  It can be multiline and multiple
 * sentences formatted to column 70.
 *
 * @param[out] arg1 output stashed here
 * @param[in] arg2 whatever
 *
 * @return 0 ...
 */
BN_EXPORT extern int bn_function(void *arg1, const void *arg2);

Yes, many of our declarations are not that pretty and clean, but most of the new ones are. We need GCI tasks to clean them all up eventually.  ;)

Also note that the "routines to handle polygons" comment is still superfluous/unnecessary.  It will get associated with bn_polygon_area only which is probably not what you intended.  I suggest just eliminating it altogether.

Lastly, const on a size_t is meaningless because the size_t is passed by value in C.  It's only when you have a reference, pointer, or array that const becomes useful.

One of the mentors has sent this task back for more work. Talk to the mentor(s) assigned to this task to satisfy the requirements needed to complete this task, submit your work again and mark the task as complete once you re-submit your work.

The work on this task is ready to be reviewed.

Congratulations, this task has been completed successfully.

If you choose to implement either, please only implement and commit one at a time per task so that there's room for discussion if needed.  Plus that might give you a little time to update all the places that should be calling the new function.