DiamondBounds documentation incorrect (was ACTS-275)

Original author Shaun Roe @sroe

The explanation in the documentation for the order of the DiamondsBounds constructor parameters appears to be incorrect. For example, instantiating a DiamondBounds with parameters

double minHalfX(30.), midHalfX(10.), maxHalfX(50.), halfY1(10.), halfY2(20.);

leads to both 'alpha' angles being 45°.

changed the description

changed milestone to %Backlog

added Bug Minor labels

changed weight to 10

assigned to @sroe and @asalzbur

assigned to @pagessin and unassigned @sroe and @asalzbur

The documentation currently reads:

/// Constructor for convex hexagon symmetric about the y axis
///
/// @param minhalex is the halflength in x at minimal y
/// @param medhalex is the halflength in x at y = 0
/// @param maxhalex is the halflength in x at maximal y
/// @param haley1 is the halflength into y < 0
/// @param haley2 is the halflength into y > 0

I think we have an image in the doc somewhere:

But I think that's wrong actually, since the mid section has zero height in the implementation. The vertex representation is written as

return {{minHalflengthX(), -halflengthY1()},
        {medHalflengthX(), 0},
        {maxHalflengthX(), halflengthY2()},
        {-maxHalflengthX(), halflengthY2()},
        {-medHalflengthX(), 0},
        {-minHalflengthX(), -halflengthY1()}};

Which corresponds to the shape in the image, with the central section being zero height. To me, that seems consistent with the doc comment.

The values given above (double minHalfX(30.), midHalfX(10.), maxHalfX(50.), halfY1(10.), halfY2(20.);) are invalid I believe, since the middle halflength in x can't be smaller than the halflengths at the extrema. This should by now actually be caught in the constructor and lead to an exception.

@sroe, do you remember what this was about spwecifically? Otherwise I'll close this.

You are right that the parameters I gave would lead to a concave hexagon instead of a convex one. If you have guarded against this, then all well and good. The diagram should reflect the fact that the DiamondBounds object represents a hexagon, not an octagon. (as stated in the documentation). Minor: Better chosen parameter names might help in the interpretation; it's an hexagonal diamond shape, and the dimensions have very specific names which I would hesitate to use all of (e.g. 'table', 'culet'), but perhaps topHalfX, midHalfX, baseHalfX, crownHeight, pavilionDepth.

Honestly, we could just go with the naming in the figure. Also, the angles are not really used (or stored) anywhere, are they?

yes, although the recurring 'h' is redundant; just go with x1, x2, x3, y1, y2. I've no idea why the angles are there also!

closed

accidentally closed the issue; but anyway I don't think there's more to discuss. If you are in the code, perhaps you could insert the new diagram. Its svg, hope thats ok.

I'll change the names and update the docs. SVG should be fine.

created branch 275-diamondbounds-documentation-incorrect-was-acts-275 to address this issue

reopened

mentioned in merge request !616 (merged)

changed milestone to %0.12.00

added Improvement label and removed Bug label