Conversation
honzikschenk
left a comment
honzikschenk
left a comment
There was a problem hiding this comment.
Initial review. Looks good so far!
| ANGLE_THRESHOLD = 0.087 # ~= 5 degrees | ||
| PARALLEL_THRESHOLD = 0.5 | ||
| bottom_corners = [] | ||
| parallel_to_x = { |
There was a problem hiding this comment.
The perpendicular walls would not be parallel to x, thus this dict should have a different naming scheme
| PARALLEL_THRESHOLD = 0.5 | ||
| bottom_corners = [] | ||
| parallel_to_x = { | ||
| "parallel1": None, # wall with cardinal direction, as first index in ordered dict |
There was a problem hiding this comment.
Please make the wall naming more obvious (parallel to what, 1/2 mean what for each side, etc)
There was a problem hiding this comment.
Additionally, you don't need the initial dict. You can just build your named vectors immediately
|
|
||
| MARGIN = 0.5 | ||
| ANGLE_THRESHOLD = 0.087 # ~= 5 degrees | ||
| PARALLEL_THRESHOLD = 0.5 |
There was a problem hiding this comment.
Please specify units, or what these measure
| "perp1": None, | ||
| "perp2": None, | ||
| "parallel2": None, | ||
| "ground": None, |
There was a problem hiding this comment.
Make the ground plane a different variable seperate from a wall dict (you can just set ground vector here)
| parallel_to_x["ground"] = i | ||
| continue | ||
| dp = np.dot(wall_normal, np.array([1, 0, 0])) | ||
| angle = math.acos(np.clip(dp / (np.linalg.norm(wall_normal) * 1), -1, 1)) |
There was a problem hiding this comment.
Don't need to * 1 but I like what you did here
| bottom_corners.append(corner2) # back corner | ||
|
|
||
| target_locations = [] | ||
| for i in targets: |
There was a problem hiding this comment.
Could you do something like for target in targets just for readability (and in any other relevant places in the code)
| target_on_wall = False | ||
| target = np.array([i.location.x, i.location.y, i.location.z]) | ||
| for j, k in zip(bottom_corners, list(parallel_to_x.values())[0:4]): | ||
| vector = np.subtract(target, j) |
There was a problem hiding this comment.
Please make these variable names clearer
| ) | ||
| if ( | ||
| angle > math.pi / 2 - ANGLE_THRESHOLD | ||
| and angle < math.pi / 2 + ANGLE_THRESHOLD |
There was a problem hiding this comment.
You can do abs(math.pi/2 - angle) < ANGLE_THRESHOLD instead
| vector[1] | ||
| ): | ||
| right, up = abs(vector[1]), abs(vector[2]) | ||
| relative_position = Coordinate(0, right, up) |
There was a problem hiding this comment.
I was thinking that for the coordinate class, we always ignore z and treat x and y as right and up (relative to the specific wall in this case). This would be easier for the groundside data parser to understand
| "parallel2": None, | ||
| "ground": None, | ||
| } | ||
| direction = ["NORTH", "EAST", "SOUTH", "WEST", "NORTH", "EAST", "SOUTH", "WEST"] |
There was a problem hiding this comment.
Mayhaps there's a better way to do this than just hard-coding directions twice (referencing line 126 as well). I'll think about this while you look at the rest of the changes, and maybe we can brainstorm more. For now though, you could just mod the references to this list by its length
2 participants
Target Localizer