Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bootcamp - Tao Shen #50

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Bootcamp - Tao Shen #50

wants to merge 1 commit into from

Conversation

ShiftyTS
Copy link

No description provided.

Copy link
Contributor

@TongguangZhang TongguangZhang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed

Comment on lines +114 to +116
for i in range(num_boxes):
result, box = bounding_box.BoundingBox.create(boxes_cpu[i])
bounding_boxes.append(box)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what if the bounding box creation fails (i.e. result is False), do we still want to append the box?

# Remove this when done
raise NotImplementedError

status = report.status
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's not reassign variables - use report.status throughout so it's easier to keep track of (and uses less memory)

Comment on lines +83 to +85
elif status == drone_status.DroneStatus.MOVING:
if self.relative_distance(relative_x_dist, relative_y_dist) < self.acceptance_radius:
command = commands.Command.create_halt_command()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this extra check (you don't have to change anything here but combining both if statements with an and would reduce the redundancy)

Comment on lines +70 to +71
status = report.status
position = report.position
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above, let's not reassign variables - use report.status and report.position throughout so it's easier to keep track of (and uses less memory)


if self.visited_waypoint: # Travel to nearest landing pad
if status == drone_status.DroneStatus.HALTED: # If halted, need to first calculate the nearest landing pad
self.nearest_landing_pad = self.get_nearest_landing_pad(position, landing_pad_locations)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

avoid calculating this twice (when you reach the landing pad you run this again)

nearest_landing_pad_distance = current_distance
return nearest_landing_pad

def get_relative_location_distance(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is actually the squared distance - let's rename this function to be more descriptive

Comment on lines +133 to +134
locOne: location.Location,
locTwo: location.Location,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we try to use snake_case, not camelCase

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants