Feat/precompute pathing#361
Conversation
miyatakazuya
left a comment
miyatakazuya
left a comment
There was a problem hiding this comment.
I left a few comments on some minor syntax issues and error handling, but overall the changes are good. The exit angle calculation although not nearly perfect does seem to be much more performant in terms nicer looking sitl paths. The PR also makes our encapsulation alot better in our ticks. I think once the comments are addressed I'll re-review really quick and it should be good to merge 🦖
| * @return angle of exit (+x-axis is 0 deg, ccw is positive) | ||
| */ | ||
| double calculateFinalAngle(const MissionPath& path, | ||
| const std::optional<CartesianConverter<GPSProtoVec>> cartesianConverter); |
There was a problem hiding this comment.
cartesianConverter can be a reference to avoid a copy
| XYZCoord pt1 = cartesianConverter->toXYZ(coords[coords.size() - 2]); | ||
| XYZCoord pt2 = cartesianConverter->toXYZ(coords[coords.size() - 1]); |
There was a problem hiding this comment.
Should cartesianConverter be optional at all? By the time PathGenTick is running the mission upload must have occured, so the converter should exist.
I think its fine to keep it optional for our sanity, but then we should check the optional has a value before dereferencing it here
|
|
||
| std::vector<GPSCoord> output_coords; | ||
| for (const XYZCoord &waypoint : path) { | ||
| output_coords.push_back(state->getCartesianConverter()->toLatLng(waypoint)); |
There was a problem hiding this comment.
unsafe optional reference with cartesianConverter (i feel like i have something against cartesian converter or smth idk why i keep spotting these
| } | ||
|
|
||
| MissionPath generateSearchPath(std::shared_ptr<MissionState> state) { | ||
| std::vector<GPSCoord> generateSearchPath(std::shared_ptr<MissionState> state, double start_angle) { |
There was a problem hiding this comment.
missing a bounds check for the waypoints? I might be misinterpreting it but why is there a bounds check in generateNextWaypointPath to check that we have at least 1 waypoint but not here?
There was a problem hiding this comment.
The number of waypoints must be greater than 1 for the code to execute generate SearchPath.
miyatakazuya
left a comment
miyatakazuya
left a comment
There was a problem hiding this comment.
The changes implemented addresses the comments well. I think optional for the cartesianConverter can be addressed at some point, but the assert solution is fine with me for now. LGTM 🦖
* feat: precompute all paths * chore: add buffer between computed paths * feat; consolidate waypoints into one mission * deleted existence of next waypoint * revert: revert past 2 commits * fix: prevent duplicate path decrement * feat: add clear mav mission * fix: change mavlink mission upload scheme * feat: unit tests + refactor code * chore: lint * chore: revert cout in gcs_routes * chore: ensure mission starts * chore: ensure that path has enough points before deleting * chore: lint * feat: assert existance of cartesean converter * chore: pass cartesean converter by reference * chore: create check for search path generation --------- Co-authored-by: Braxten Nguyen <braxtennguyen556@gmail.com>
3 participants
closes #342 #350 #360... sorry for the bloat
Changes
mission_state::next_waypoint_pathand associated functions, which is the waypoint-->waypoint pathpath_gennow precomputes all new paths (they must be in sequence now)fly_waypointsno longer generates new pathsstatic::calculateFinalAngle()to rougly approximate the angle of an existing pathcalculateFinalAngleTesting
TODO