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

Completed Bootcamp #128

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Completed Bootcamp #128

wants to merge 4 commits into from

Conversation

lilyjge
Copy link

@lilyjge lilyjge commented Nov 24, 2024

No description provided.

modules/bootcamp/decision_simple_waypoint.py Outdated Show resolved Hide resolved
Comment on lines 122 to 124
result, box = bounding_box.BoundingBox.create(boxes_cpu[i])
if result:
bounding_boxes.append(box)

Choose a reason for hiding this comment

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

its common practice to disregard the entire inference if there is any result

Copy link
Author

Choose a reason for hiding this comment

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

Could you explain more? Why should the inference be disregarded if there is an result? What's the point of having a list of boxes then? Thank you!

Comment on lines 119 to 120
else:
command = commands.Command.create_land_command()

Choose a reason for hiding this comment

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

the intended solution is to add another is_same check here with the landing pad location. in fact this one matters a lot more than the previous check as you need to be physically landing on a landing pad

@@ -69,6 +79,17 @@ def run(
# ============

# Do something based on the report and the state of this class...
if report.status.name == "HALTED":
Copy link

@AaronWang04 AaronWang04 Nov 28, 2024

Choose a reason for hiding this comment

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

I wouldnt usually do equality by name here, you can directly associate with class type. but you don't have to make the change here since it would still work

Comment on lines +50 to +54
return (
abs(position.location_x - self.waypoint.location_x) ** 2
+ abs(position.location_y - self.waypoint.location_y) ** 2
< self.acceptance_radius**2
)

Choose a reason for hiding this comment

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

good job not using sqrt to reduce computation

@@ -38,11 +38,36 @@ def __init__(self, waypoint: location.Location, acceptance_radius: float) -> Non
# ============

# Add your own
self.closest_landing_pad = location.Location(0, 0)

Choose a reason for hiding this comment

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

initialization with none is probably better convention, the value of location(0,0) will never be used

command = commands.Command.create_land_command()
elif report.status.name == "MOVING":
# At waypoint OR waypoint has been reached and at landing pad.
if self.is_same(self.waypoint, report.position) or (

Choose a reason for hiding this comment

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

this wouldnt work as the drone may never leave the waypoint.

the goal is for the drone to arrive at the waypoint, snap a picture to find landing pads, and move to an apprioate landing pad to land on.

I think elif moving: pass is fine here

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