Draft
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.
adamkewley
reviewed
Jul 20, 2023
| double ATan2OnPositiveRange(const SimTK::Vec2 point) | ||
| { | ||
| const double angle = std::atan2(point[1], point[0]); | ||
| return angle < 0.? angle + c_TAU : angle; |
Collaborator
There was a problem hiding this comment.
Reformat: expr ? true : false (space before ?)
| return angle < 0.? angle + c_TAU : angle; | ||
| } | ||
|
|
||
| class Angle final { |
Collaborator
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) : |
Collaborator
There was a problem hiding this comment.
explicit (note: this ctor does enforce the [0..2pi] class invariant)
| Angle(double angleChecked): _value(angleChecked) | ||
| {} | ||
|
|
||
| public: |
Collaborator
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. |
Collaborator
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>> |
Collaborator
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]( |
Collaborator
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. |
Collaborator
There was a problem hiding this comment.
Stoppped reading around here - no time
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Refactors the cylinder wrapping math.
Still need to iron out some details, so it is more about the overall structure.