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

Daniel Suzuki's Bootcamp #97

Closed
wants to merge 18 commits into from
Closed

Daniel Suzuki's Bootcamp #97

wants to merge 18 commits into from

Conversation

dasuzuki1
Copy link

No description provided.

Copy link
Member

@maxlou05 maxlou05 left a comment

Choose a reason for hiding this comment

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

Reviewed

self.command_index = 0
self.commands = [
commands.Command.create_set_relative_destination_command(
waypoint.location_x, waypoint.location_y
Copy link
Member

Choose a reason for hiding this comment

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

This command uses relative distance, so your position matters. You may end up going somewhere you didn't intend to.

report.status == drone_status.DroneStatus.HALTED
and (
(report.position.location_x - self.waypoint.location_x) ** 2
+ (report.position.location_y - self.waypoint.location_y)
Copy link
Member

Choose a reason for hiding this comment

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

I think you forgot to square the delta y

):
command = commands.Command.create_land_command()

self.has_sent_landing_command = True
Copy link
Member

Choose a reason for hiding this comment

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

This variable isn't really used for anything, so you can delete it

@@ -67,8 +70,55 @@ def run(
# ============
# ↓ BOOTCAMPERS MODIFY BELOW THIS COMMENT ↓
# ============
def square(number: int) -> float:
Copy link
Member

Choose a reason for hiding this comment

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

number probably should be a float

return number**2

def squared_distance_from_position(
point: list[location.Location], current_position: list[location.Location]
Copy link
Member

Choose a reason for hiding this comment

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

point and position should probably just be 1 location, and not a list of locations

if self.halt_at_initialization:

command = commands.Command.create_set_relative_destination_command(
self.waypoint.location_x, self.waypoint.location_y
Copy link
Member

Choose a reason for hiding this comment

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

This is a relative distance comamnd, please take into consideration your own position

Comment on lines 99 to 106
squared_distance_from_position(landing_pad, self.waypoint)
< self.shortest_distance[0] ** 2 + self.shortest_distance[1] ** 2
):

self.shortest_distance[0], self.shortest_distance[1] = (
landing_pad.location_x - report.position.location_x,
landing_pad.location_y - report.position.location_y,
)
Copy link
Member

Choose a reason for hiding this comment

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

You can probably just store the distance squared instead of both dx and dy and calculating the distance again many times

self.closest_pad = landing_pad

command = commands.Command.create_set_relative_destination_command(
self.shortest_distance[0], self.shortest_distance[1]
Copy link
Member

Choose a reason for hiding this comment

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

Because here, you can just use self.closest_pad, which is a Location (has its own x and y coords)

Comment on lines 114 to 118
elif (report.position.location_x**2 + report.position.location_y**2) / (
self.closest_pad.location_x**2 + self.closest_pad.location_y**2
) <= (
(1 + self.acceptance_radius) ** 2
): # checks if current position is in acceptable radius of landing pad by making them a ratio
Copy link
Member

Choose a reason for hiding this comment

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

Not sure why you calculated like this ratio? You can just use the same way you did earlier with the comparing your distance to acceptance_radius

Copy link
Member

@maxlou05 maxlou05 left a comment

Choose a reason for hiding this comment

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

Reviewed

@@ -37,7 +37,8 @@ def __init__(self, waypoint: location.Location, acceptance_radius: float) -> Non
# ↓ BOOTCAMPERS MODIFY BELOW THIS COMMENT ↓
# ============

# Add your own
self.halt_at_initialization = True
self.has_sent_landing_command = False
Copy link
Member

Choose a reason for hiding this comment

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

You can delete this here too

Comment on lines 114 to 117
elif (
(report.position.location_x - self.closest_pad.location_x) ** 2
+ (report.position.location_y - self.closest_pad.location_y) ** 2
) <= ((self.acceptance_radius) ** 2):
Copy link
Member

Choose a reason for hiding this comment

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

I think your brackets are wrong here, your elif ends before the <= operator

Copy link
Author

Choose a reason for hiding this comment

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

I've realized that when i use black modules , the brackets that I add at the front of 'elif' and after '**2' end are removed. Is there any way to resolve this?

Comment on lines 102 to 104
current_shortest_distance = (
self.closest_pad.location_x - report.position.location_x
) ** 2 + (self.closest_pad.location_y - report.position.location_y) ** 2
Copy link
Member

Choose a reason for hiding this comment

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

You can use your square_distance_... function here too

@@ -36,12 +36,16 @@ def __init__(self, waypoint: location.Location, acceptance_radius: float) -> Non
# ============
# ↓ BOOTCAMPERS MODIFY BELOW THIS COMMENT ↓
# ============
# set as closest landing to waypoint
self.has_sent_landing_command = False
Copy link
Member

Choose a reason for hiding this comment

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

Probably don't need this one either

Copy link
Member

@maxlou05 maxlou05 left a comment

Choose a reason for hiding this comment

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

Approved!

@maxlou05 maxlou05 closed this Sep 15, 2024
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