#949: Setting Goal Yaw#959
Conversation
…/UBCSailbot/sailbot_workspace into raghumanimehta/some-viz-upgrades merge
…into ethanhu912/949-path-set-goal-yaw
ethanhu912
requested review from
a team,
FireBoyAJ24 and
raghumanimehta
as code owners
There was a problem hiding this comment.
Well done @ethanhu912 ! This was good work. I had some problems with readibility and that is why i am requesting changes but this is a high quality contribution.
Please update the tests when you add the buffer near NO_GO_ZONE in _offset_if_in_irons.
| for i in range(1, len(waypoints)): | ||
| if waypoints[i] == reference: | ||
| # reference_latlon sits at (0, 0) in the OMPL frame | ||
| next_to_next_xy = cs.latlon_to_xy(reference, waypoints[i - 1]) |
There was a problem hiding this comment.
| next_to_next_xy = cs.latlon_to_xy(reference, waypoints[i - 1]) | |
| next_to_next_gw_xy = cs.latlon_to_xy(reference, waypoints[i - 1]) |
| if waypoints[i] == reference: | ||
| # reference_latlon sits at (0, 0) in the OMPL frame | ||
| next_to_next_xy = cs.latlon_to_xy(reference, waypoints[i - 1]) | ||
| bearing_deg = cs.get_path_segment_true_bearing(cs.XY(0, 0), next_to_next_xy) |
There was a problem hiding this comment.
nit: You should directly refer reference_xy to improve readibility rather than cs.XY(0, 0).
not nit: shouldn't the order of argument be reversed here?
There was a problem hiding this comment.
Please pushback if i am wrong here.
There was a problem hiding this comment.
Nevermind, the confusion is resolved. I would ask for better naming of variables and a small comment clarifying that reference is the next_gw_xy and that it is calculating a bearing from this to next to next.
| ) | ||
| return self._offset_if_in_irons(0.0) | ||
|
|
||
| def _offset_if_in_irons(self, ompl_yaw_rad: float) -> float: |
There was a problem hiding this comment.
rename to include rad in the name as you did for the others.
| ) | ||
| return self._offset_if_in_irons(0.0) | ||
|
|
||
| def _offset_if_in_irons(self, ompl_yaw_rad: float) -> float: |
There was a problem hiding this comment.
rename ompl_yaw_rad to target_yaw_rad. Seems clearer to me. Thoughts?
There was a problem hiding this comment.
Agreed. Will change right now
| ompl_yaw_rad: Proposed goal yaw in OMPL Cartesian radians. | ||
|
|
||
| Returns: | ||
| float: A goal yaw in OMPL Cartesian radians outside the wind no-go zone. |
There was a problem hiding this comment.
Should specify what the OMPL Cartesian coordinates are for readibility.
|
|
||
| if abs(twa) < NO_GO_ZONE: | ||
| # Upwind irons: snap to the closer ±NO_GO_ZONE edge. | ||
| new_twa = NO_GO_ZONE if twa >= 0 else -NO_GO_ZONE |
There was a problem hiding this comment.
We shouldn't be setting it eactly to NO_GO_ZONE. Consider adding a small margin to make sure OMPL doesn't crash because of validitiy not passing.
| goal-yaw helpers read. Avoids running the OMPL solver in every test. | ||
| """ | ||
| wind = Wind(tw_speed_kmph, tw_dir_deg) | ||
| state = SimpleNamespace( |
There was a problem hiding this comment.
nit: consider using Mock instead of SimpleNamespace.
| fake._offset_if_in_irons = types.MethodType(ompl_path.OMPLPath._offset_if_in_irons, fake) | ||
| return fake | ||
|
|
||
|
|
There was a problem hiding this comment.
In the test file, why not use pytest mark parametrize?
There was a problem hiding this comment.
Never mind this, I got codex to care of the problems in tests. Focus on ohter comments.
…readability + margin on no-go zone offset.
3 participants
Description
Verification
Resources
ompl_path.py