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

Added the target heading and distance to the compass when it is set #68

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

Conversation

Geeoon
Copy link
Member

@Geeoon Geeoon commented Apr 16, 2024

After pressing "Set Waypoint" in the Waypoint Navigation interface, the heading to the target will appear on the compass as a gold circle (i.e., when the needle is facing the gold circle, it is facing the target). And the distance to the target will be displayed on the compass (>20 meters or the actual value rounded to the nearest tenth). The "Set Waypoint," button turns into "Unset Waypoint" after the waypoint has been set, and at that point, the "Go" button will be enabled. Pressing "Go" will disable the waypoint inputs and buttons.

@Geeoon Geeoon requested a review from abhaybd April 16, 2024 02:02
@abhaybd abhaybd requested a review from huttongrabiel April 18, 2024 00:06
@huttongrabiel
Copy link
Contributor

My JS skills are pretty bad tbh but I'll take a closer look at the code when I get a chance. Functionality wise it looks correct from the testing I've done. I'll actually review this correctly in the next few days.

Copy link
Contributor

@huttongrabiel huttongrabiel left a comment

Choose a reason for hiding this comment

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

LGTM. Nothing stands out. @abhaybd can you take a look over this? Testing wise it all works really nicely.

@Geeoon Geeoon requested a review from huttongrabiel May 14, 2024 02:06
@Geeoon Geeoon changed the title Added the target heading to the compass when it is set Added the target heading and distance to the compass when it is set May 14, 2024
Copy link
Contributor

@huttongrabiel huttongrabiel left a comment

Choose a reason for hiding this comment

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

nice! lgtm @abhaybd ready for your final pass to catch js things i miss

@Geeoon Geeoon requested a review from imisaacwu May 24, 2024 00:51
Copy link
Contributor

@imisaacwu imisaacwu left a comment

Choose a reason for hiding this comment

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

Not connected to rover: Waypoint can be set (0,0) is directly East, no distance labels
Maybe disallow setting of waypoint when there's no rover?

Copy link
Member

@abhaybd abhaybd left a comment

Choose a reason for hiding this comment

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

This looks good, nice job! Just left some minor notes.

Also Isaac raised a good point, when the distance is <0.5m or something we shouldn't display a heading indicator (maybe display like a green ring around the outside of the compass to indicate we've reached? you tell me)

I think setting the waypoint is fine when the rover is disconnected, we should just disable the "start navigation" button.

@@ -32,6 +33,53 @@ function sanitize(num, decimals) {
return num >= 0 ? " " + ret : ret;
}

/**
* Convert latitude and longitudes to heading.
* Based on https://www.igismap.com/formula-to-find-bearing-or-heading-angle-between-two-points-latitude-longitude/
Copy link
Member

Choose a reason for hiding this comment

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

Kinda nitpicky, but can we make this @see https://...


/**
* Convert latitude and longitudes to distance.
* Based on the Haversine formula (https://en.wikipedia.org/wiki/Haversine_formula)
Copy link
Member

Choose a reason for hiding this comment

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

Same here, do @see https://...

@Geeoon
Copy link
Member Author

Geeoon commented Jun 28, 2024

Also Isaac raised a good point, when the distance is <0.5m or something we shouldn't display a heading indicator (maybe display like a green ring around the outside of the compass to indicate we've reached? you tell me)

I opted to change the color of the target distance text. The outer compass color is already used to indicate the attitude of the rover.

@Geeoon Geeoon requested a review from abhaybd June 28, 2024 08:59
@Geeoon Geeoon force-pushed the target-improvements branch from 04a457b to 0ff4659 Compare July 18, 2024 07:42
@abhaybd
Copy link
Member

abhaybd commented Aug 15, 2024

Didn't get to this before CIRC, Hutton can handle review and merging.

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.

4 participants