Cylinder wrap math #13
Conversation
Adds unconstrained quadrant, which is always valid. Writes conditions to assert ifNotValid instead.
The long and short side of right angled triangle was flipped.
| double ATan2OnPositiveRange(const SimTK::Vec2 point) | ||
| { | ||
| const double angle = std::atan2(point[1], point[0]); | ||
| return angle < 0.? angle + c_TAU : angle; |
There was a problem hiding this comment.
Reformat: expr ? true : false (space before ?)
| return angle < 0.? angle + c_TAU : angle; | ||
| } | ||
|
|
||
| class Angle final { |
There was a problem hiding this comment.
Design vs. usage vary.
- If the intention is that
getValuealways returns a value in the range[0..2pi]then thedoublector should enforce the fmod-like behavior, or have aUnsafe_AngleAlreadyKnownToBeInRangetag argument to indicate to downstream code that the caller "knows what they're doing" - If the only reason to have the
doubleargument is to facilitateoperator+then you should make thedoubleconstructor private andfriend Angle operator+(Angle, Angle);, so that the operator is allowed to skip the invariant check (this is what has been done, but I'm guessing the ctor is public for another reason) - If any downstream code should be able to provide any value then the ctor should
fmodthe argument NaNforms a 2nd form of the state (i.e. "bad state"). Ensure that's relevant (e.g. rather than "identity" -0.0)
|
|
||
| class Angle final { | ||
| // Construct from angle already on range [0...2pi]. | ||
| Angle(double angleChecked): _value(angleChecked) |
| public: | ||
| Angle() = default; | ||
|
|
||
| Angle(const SimTK::Vec2& point) : |
There was a problem hiding this comment.
explicit (note: this ctor does enforce the [0..2pi] class invariant)
| Angle(double angleChecked): _value(angleChecked) | ||
| {} | ||
|
|
||
| public: |
There was a problem hiding this comment.
public: indentation usually matches class, not the same level as the things under public scope
| // wrapping path on a circle. | ||
| namespace { | ||
|
|
||
| // Struct for holding a generic pair, that differ in the wrapping direction. |
There was a problem hiding this comment.
You had better have a very good reason to be using templating, std::function, rvalue semantics (&&), and varadaic templates in one class here.
|
|
||
| // Computes for both positive and negative wrapping directions, the tangent | ||
| // line path segments. | ||
| PositiveAndNegativeRotatingPair<PathSegment<TangentLine>> |
There was a problem hiding this comment.
The templating is getting a bit OTT here
| const PathSegment<BothTangentLines> tangentLines(path, circleRadius); | ||
|
|
||
| // Get one of the possible paths by setting the rotation direction. | ||
| auto GetPathSegmentsGivenDirection = [&tangentLines]( |
There was a problem hiding this comment.
The payoff for std::function, forwarding, etc. did not justify this bit - just compute it and pass it into a simpler struct/class imo.
| CircleWrap TakeBestPath( | ||
| PositiveAndNegativeRotatingPair<CircleWrap> possibleWrappings) | ||
| { | ||
| return TakeBestPath(possibleWrappings.positive, |
| possibleWrappings.negative); | ||
| } | ||
|
|
||
| // Computes the best wrapping path on a circle. |
There was a problem hiding this comment.
Stoppped reading around here - no time
Refactors the cylinder wrapping math.
Still need to iron out some details, so it is more about the overall structure.