Added class CollisionEvaluator to simplify TrajectoryPlanner #3547
Conversation
StarrryNight
requested review from
Andrewyx,
sauravbanna and
williamckha
as code owners
| // Avoid finding the first non-collision time if the cache sub-trajectory | ||
| // has a collision time. | ||
| if (sub_traj_with_cost.has_value() && | ||
| sub_traj_with_cost->collision_duration_front_s < sub_traj_duration_s) |
There was a problem hiding this comment.
since sub_traj_duration_s is an optional, does it need either dereference operator or .value()? (though I assume if it compiles, it automatically does this so yeah)
adrianchan787
left a comment
adrianchan787
left a comment
There was a problem hiding this comment.
Issue itself looks great, also tried to find improvements in the collision code but couldn't find any. Added a few comments, though they are extremely small things
| * @param traj_path The trajectory path to check | ||
| * @param start_time_s The time in seconds to start the search from | ||
| * @param search_end_time_s The time in seconds to stop the search at | ||
| * @return The first collision time within [start_time_sec and search_end_time_s] |
There was a problem hiding this comment.
really really really nitpicky and useless comment, maybe spec should include mention of COLLISION_CHECK_STEP_INTERVAL_SEC. I believe "the first collision time within [start time sec and search end time s] is technically incorrect, maybe something like "the first collision time within [start time sec and search end time s] using a COLLISION_CHECK_STEP_INTERVAL_SEC resolution"? (super super nitpicky though, probably not worth changing)
2 participants
Description
This PR resolves issue #3546, TrajectoryPlanner::getTrajectoryWithCost can be cleaned up by delegating the task of either using a cached collision value or a new one to a separate class. It is a subissue of #3104 .
#3546
The main task of this ticket was to simplify TrajectoryPlanner:getTrajectoryWithCost by delegating the evaluation of collision values to a new class -- CollisionEvaluator. The old logic was preserved, but the code is more modular.
Testing Done
All test cases for TrajectoryPlanner passed. I dd not write new test cases for the new class separately as the class should be well tested by TrajectoryPlanner for now. However, I think it will be better to have a TestTrajectoryPlanner file for some individual tests, just in case CollisionEvaluator is used somewhere else. I will open a new issue for this.
Resolved Issues
#3546
Length Justification and Key Files to Review
Review Checklist
It is the reviewers responsibility to also make sure every item here has been covered
.hfile) should have a javadoc style comment at the start of them. For examples, see the functions defined inthunderbots/software/geom. Similarly, all classes should have an associated Javadoc comment explaining the purpose of the class.TODO(or similar) statements should either be completed or associated with a github issue