-
Notifications
You must be signed in to change notification settings - Fork 24
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
Improve UX of the mission planning pipeline #1469
Improve UX of the mission planning pipeline #1469
Conversation
c63d7c4
to
59d7919
Compare
@ArturoManzoli since the two first commits are unrelated to the mission planning part, could you open dedicated PRs for them? This way we can test them separately and merge sooner. This will also make the review and rebase processes easier. |
700af16
to
1616067
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice improvements!
I will add some points from my usage tests here.
- Could we make the survey path follow the angle change live, as it was before? It helps specially with surveys with concavities where the user needs to know if the change in the survey angle will make it go outside the survey polygon.
Kapture.2024-12-05.at.06.12.11.mp4
- The survey angle change step should be 0.1 degrees. It's currently not limited.
- I cannot select the regular mission path waypoints to see them, and so I cannot see information around them.
Kapture.2024-12-05.at.06.15.19.mp4
- If I have more than one survey, I cannot edit them. Is this expected? Once I removed one of them, I could edit the other again.
Kapture.2024-12-05.at.06.17.43.mp4
-
"Scan density" should be renamed to something like "Scan spacing (m)", as the density is actually decreasing as the number increases.
-
I couldn't find the button to delete all waypoints. Where is it?
-
About Allow user to specify the angle of attack for a survey mission #1350, is this PR changing something on the logic related to the survey angle?
Will take a better look at the code and put notes at a separate review.
Very nice new features! looking forward to get them integrated! |
1616067
to
9f76bcd
Compare
Didn't get what you mean here. Are you referring to the dashed lines that preview the survey scan path? Because they do update live.
Yes, it did implement the feature of selecting and changing the 'angle of attack' for a survey. Although, we did decided later on to not call it 'angle of attack' because of the existing aeronautical term. |
As discussed on a meeting, the idea here is to the path angle to change live while the user is dragging the arrow, instead of needing to release and press it repeatedly to archive the desired path. |
9f76bcd
to
603180e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some thoughts around the code:
-
This PR has around 8 big changes. It is important to separate those into several commits during development. It is really hard to review one big commit with 2 thousand lines, and it's also difficult to make sure nothing is broken during the review process, as all changes are made to the same commit.
-
When moving a file that is also being modified in the same commit, we lose the diff completely. This is what is happening with the
MissionPlanningView.vue
file. There's no way for those reviewing the code to understand what is being changed. This is specially a problem here where the file had 700 lines added. My suggestion is that this file remains where it was, as it is the base view file, and move theContextMenu.vue
andScanDirectionDial.vue
to a subfolder in thesrc/components
folder instead. -
The provide/inject functionality was added in Vue to make it easier to deal with with prop drilling. This is not the case here, since the
ContextMenu
is a direct child ofMissionPlanningView
. We should use props and events instead.
f7861ca
to
9d18306
Compare
9d18306
to
e8c7b59
Compare
e8c7b59
to
5ae6f43
Compare
5ae6f43
to
6eb3ecf
Compare
Yep, the context menu already had the correct text. Changing the button to match it |
40fdcd9
to
d31738b
Compare
3c4b4e5
to
89259e1
Compare
On the last push:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems there's a problem with the commits:
- The first commit has the title "Mission-planning: Add context-menu component", but it is actually adding the entire functionality of the PR
- The second commit says it's going to "Add scan direction modifier to existent surveys", but it end ups rewriting a lot of functionality from the first commit, and also implementing functionality from commits 3 and 4.
- Commit 3 says it's adding the undo functionality but is actually disabling dragging, which is them re-enabled in the last commit.
I think a good separation, that would make your life easier, is to have the first commit only creating the ScanDirectionDial component, and the second commit only creating the ContextMenu component (literally only adding the components files, since they didn't exist yet, but not importing those components anywhere, to not complicate things), and leave the importing/implementation of the functionalities to subsequent commits. This way I think you shouldn't have problems with the rebase process.
@@ -53,16 +55,16 @@ watch( | |||
} | |||
) | |||
|
|||
const surveyLinesAngle = inject<(angle: number) => void>('surveyLinesAngle') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I see now that what is happening is that you added the functionality in the first commit to remove it later in this one. This should be fixed. I will put a suggestion in the review comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, thanks for letting me know. I'll restructure the commits. They were tidy until last rebase. I'll check what happened
Signed-off-by: Arturo Manzoli <[email protected]>
89259e1
to
3d8fea6
Compare
Signed-off-by: Arturo Manzoli <[email protected]> squash to ScanDirectionDial Signed-off-by: Arturo Manzoli <[email protected]>
db453fa
to
e1a39ff
Compare
Signed-off-by: Arturo Manzoli <[email protected]>
Signed-off-by: Arturo Manzoli <[email protected]>
e1a39ff
to
e5611e4
Compare
New Features List:
Screenshare.-.2024-11-26.4_37_36.PM.mp4
Screenshare.-.2024-11-26.5_44_58.PM.mp4
closes #1355
closes #1350