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

UI Improvement and Features #705

Merged
merged 16 commits into from
Sep 30, 2024
Merged

UI Improvement and Features #705

merged 16 commits into from
Sep 30, 2024

Conversation

WindingMotor
Copy link
Contributor

UI Improvement and Features

This PR adds significant UI changes and several new features to improve the user experience and functionality.

New Features:

  • Path Length Feedback: Added real-time feedback in RuntimeDisplay to indicate path length changes.
  • 0.5m x 0.5m Grid Option: New grid option available in Editor Settings.
  • Robot Details Display: Added section in Editor Settings showing current robot angle and position.
  • Enhanced Tree Card Visuals: Added icons to tree cards and robot path items for better visual distinction.
  • Mini Info Cards: Introduced quick-glance information for rotation and event marker tree cards.
  • Manual Entry for Markers: Enabled manual position entry for event markers and rotation targets.
  • Wait Time Command Controls: Added increment/decrement buttons for wait time commands.

UI Improvements:

  • Redesigned Sidebar: Reworked layout for a more compact and efficient design.
  • Top Bar Optimization: Decreased overall size for better screen real estate usage.
  • Event Marker Visuals: Enhanced appearance on the path.
  • Icon Updates: Improved icon choices and colors for clarity and consistency.
  • Font Size Adjustments: Reduced font size for checkbox labels to improve readability.
  • Button Refinements: Streamlined various buttons and relocated others for better usability.
  • Replay Bar Enhancements: Improved layout and added a replay button.
  • Holonomic Mode Indicator: Added visual indicator for holonomic mode status.
  • Custom Color Scheme: Enhanced theming for greater visual consistency.
  • Lock Animation: Added animation effects for path editor locking.
  • Constraint Zone Visibility: Increased stroke thickness for better visibility.
  • Additional Tooltips: Expanded tooltip coverage for improved user guidance.
  • Arrow Key Functionality: Added 45-degree increment/decrement for rotation target entry using arrow keys.
  • Graph Enhancements: Improved visuals and added hover information.
  • Loading Experience: Added helpful checklist and IP status during loading.
  • Sidebar icon updates much better than before

As this is my first public pull request, I would greatly appreciate feedback. I've also included several before/after images demonstrating the UI changes.

Before & After Images

Path Editor

PathEditorBEFORE

PathEditorAFTER

Sidebar

ProjectBrowserBEFORE

ProjectBrowserAFTER

Telemetry

TelemetryBEFORE

TelemetryAFTER

Grid & Robot Details

PathEditorGridAndRobotDetails

Waypoints Tree

PathEditorWaypointsBEFORE

PathEditorWaypointsAFTER

Rotation Target Tree

PathEditorRotationTargetsBEFORE

PathEditorRotationTargetsAFTER


@github-actions github-actions bot added the GUI Changes to the PathPlanner GUI label Aug 18, 2024
@mjansen4857
Copy link
Owner

A lot of this looks really awesome, thanks! Just gonna have to let this sit here for a little bit though as I have another PR that has a bunch of stuff I need to get merged first before I can work on or merge anything else. I don't think it will break too much here though.

Once I get that stuff finished, tested, and merged I can come back to review this. Sorry for the delay.

@mjansen4857
Copy link
Owner

Ok that PR is merged so you can take a look at those merge conflicts. I haven't reviewed in depth but from an overview perspective there's a few things I have feedback on.

  • The side drawer should remain an actual drawer with the same styling it had previously. Its current usage follows the material design guidelines which helps keep the whole app feeling cohesive. I think a way to improve the current design would be to make the telemetry icon change that you did, by making the icon for the telemetry page the connection status indicator. Then, you could move the change project button down in to the corner that the indicator used to be in to clean things up a bit.
  • I don't think the car icon and the arrow instead of the dot for the robot preview are necessary. Once you merge you'll see that swerve modules get represented now so the car icon isn't needed. Also, keeping the dot instead of the arrow just keeps all the representations of the robot looking similar.
  • I think the graph changes on the telemetry page are mostly fine, but I think the changes to include axis labels and the different grid make it look more cluttered. I think leaving the grid/axis as-is would look a lot cleaner and having the ability to mouse over to see a value would satisfy anyone who wants to know actual values. At the end of the day, AdvantageScope is a better tool for this anyways, this is just a quick and dirty way to see that telemetry for people who don't use it.
  • Somewhat minor thing but I think the extra info display in the goal end state should also show the velocity.
  • I'm not sure if there's really a purpose in having that holonomic mode indicator. Users should know which mode they are in based on the look and function of the editor.
  • I think the add folder/path buttons in the project browser should remain filled icon buttons with the different colors. It just gives it some more contrast and draws people to them. Making the add path button less visible could cause confusion for some people since its not as obvious how they can create a new path.

Overall, the rest looks pretty great.

@WindingMotor
Copy link
Contributor Author

Ill take a look at the merge conflicts and make the changes you suggested. Thanks for the feedback!

@github-actions github-actions bot added the dependencies Pull requests that update a dependency file label Sep 8, 2024
@WindingMotor
Copy link
Contributor Author

I have merged and made the improvements you mentioned earlier. I have also fixed the app icon not working on Linux systems and added a path/auto search bars on the Project Browser. I'm also loving the new swerve modules visualization!
Here are some images for reference:

Project Browser:
image

Side Drawer:
image

Telemetry Page:
image

Path Editor:
image

@mjansen4857
Copy link
Owner

mjansen4857 commented Sep 9, 2024

Looking good. Can you add tests for the new stuff you added? Most things you touched should already be covered, but testing the search bars, the extra info displays in the path tree, the position text field for event markers/rotation targets, etc. would be great. There's lots of examples in the tests already there so it should hopefully be easy to figure out but if you need any help let me know.

It also looks like a few tests are failing so you'll need to look into why they're not working.

@WindingMotor
Copy link
Contributor Author

Sounds good, I will get to work on those tests.

@WindingMotor
Copy link
Contributor Author

After taking a quick look it appears .mocks.dart files are included in .gitignore. So my flutter files are throwing errors. I am new to using tests in flutter so any help would be appreciated, thanks!

image

@mjansen4857
Copy link
Owner

The mocks need to be generated using dart run build_runner build. You only have to do this once unless they get changed.

@WindingMotor
Copy link
Contributor Author

It took a few days but i have merged with your recent commits and updated all the old tests while adding the new ones. I have also made a few UI changes.

Project page:
image

Failed to generate snackbar:
image

Copy link

codecov bot commented Sep 15, 2024

Codecov Report

Attention: Patch coverage is 76.53167% with 226 lines in your changes missing coverage. Please review.

Project coverage is 84.38%. Comparing base (90ddfcd) to head (e0ec354).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
lib/pages/home_page.dart 3.57% 81 Missing ⚠️
lib/pages/project/project_page.dart 75.24% 51 Missing ⚠️
...ts/editor/tree_widgets/path_optimization_tree.dart 41.53% 38 Missing ⚠️
lib/pages/telemetry_page.dart 80.51% 15 Missing ⚠️
lib/widgets/editor/split_auto_editor.dart 45.83% 13 Missing ⚠️
lib/widgets/editor/split_path_editor.dart 25.00% 12 Missing ⚠️
lib/widgets/editor/tree_widgets/path_tree.dart 93.33% 9 Missing ⚠️
lib/widgets/editor/runtime_display.dart 88.00% 6 Missing ⚠️
lib/widgets/editor/path_painter.dart 94.73% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #705      +/-   ##
==========================================
- Coverage   84.72%   84.38%   -0.34%     
==========================================
  Files          86       88       +2     
  Lines        7534     7890     +356     
==========================================
+ Hits         6383     6658     +275     
- Misses       1151     1232      +81     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mjansen4857
Copy link
Owner

It looks like a lot of coverage is missing for some of the tests. From looking at the coverage report it looks like most of it was previously covered before this PR but now isn't. Perhaps you deleted some tests and never added them back? telemetry_page.dart specifically looks like it lost a lot of coverage. Try seeing if you can get those tests back and we'll see what the project coverage looks like after that. I usually try to aim for not letting the total project coverage decrease.

Copy link
Owner

@mjansen4857 mjansen4857 left a comment

Choose a reason for hiding this comment

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

Still need to actually run the build and see if I have any feedback for how it looks/works, but from a code perspective it looks pretty good besides a few minor suggestions.

lib/pages/project/project_page.dart Outdated Show resolved Hide resolved
lib/pages/project/project_page.dart Outdated Show resolved Hide resolved
lib/pages/nav_grid_page.dart Outdated Show resolved Hide resolved
lib/widgets/editor/path_painter.dart Outdated Show resolved Hide resolved
lib/widgets/editor/path_painter.dart Outdated Show resolved Hide resolved
lib/widgets/editor/tree_widgets/editor_settings_tree.dart Outdated Show resolved Hide resolved
lib/widgets/editor/tree_widgets/path_tree.dart Outdated Show resolved Hide resolved
lib/widgets/editor/tree_widgets/tree_card_node.dart Outdated Show resolved Hide resolved
lib/widgets/editor/tree_widgets/waypoints_tree.dart Outdated Show resolved Hide resolved
lib/widgets/my_application.cc Outdated Show resolved Hide resolved
@WindingMotor
Copy link
Contributor Author

I will go ahead and take a look at your suggestions and codecov's report. Hopefully we can get that coverage back up.

@WindingMotor
Copy link
Contributor Author

A test slipped one of my checks. Ill get to work on it and try to improve coverage.

@WindingMotor
Copy link
Contributor Author

Alright i made some improvements to the tests on the path tree, telemetry page, and waypoints tree.

@mjansen4857
Copy link
Owner

mjansen4857 commented Sep 19, 2024

So I think a couple things are still hurting your coverage:

  1. In the telemetry page tests you never give it data to display when connected. See the lines that were removed here
  2. I'm not 100% sure but I think sharing one telemetry mock across all tests can cause issues because the same object will get different mock responses applied to it by all the tests. You should change that back to each telemetry test creating its own telemetry object.
  3. The 'showDetails' flag for drawing the robot outline in path_painter_util is always false in tests. Set the initial setting value for showing details to true in split_path_editor_test. You'll see the block of settings stuff at the end of the setUp method.

Addressing those things should probably be enough to get the coverage back up to a good level.
Edit: Also, there's an unused import causing the analysis to fail.

@WindingMotor
Copy link
Contributor Author

Okay, i updated the telemetry page test and my local test coverage says its pretty good. I've also fixed the showDetails flag.

@mjansen4857
Copy link
Owner

mjansen4857 commented Sep 20, 2024

Ok so the fix I gave you for painting path details didn't work because in tests it never gets a trajectory generated because that happens asynchronously. The real fix here is to add that default setting to true in split_choreo_path_editor_test since that will actually draw trajectories. I've verified that this actually does work.

Also just noticed a few other coverage related things:

  1. Tests are missing for the text box input for the sliders in the event marker/rotation target trees. Definitely something that should be covered and pretty easy to test so that will help.
  2. The path painter method that paints the grid isn't covered, just setting the show grid setting to true in split_path_editor_test/split_choreo_path_editor_test will get that.
  3. The logic for adding folders/project items used to be covered in project_page but no longer is. The same logic from the old tests should be able to cover that here. Basically: tap the add button, check that it added a folder or project card.

Sorry for making you chase coverage so much lol. But having things properly covered has saved me from breaking things so many times so I think its very important.

@WindingMotor
Copy link
Contributor Author

Its alright lol

@WindingMotor
Copy link
Contributor Author

I've been very busy this past week so sorry for the delay. Most of the changes are almost implemented and i will take a look at the PR mentioned.

@mjansen4857
Copy link
Owner

No rush. Just as a heads up I'm currently working on a PR to update the Writerside documentation for all the changes I made but haven't documented yet. After that's merged you should update the docs for your changes in this PR. All you should have to do is update a few screenshots and do quick descriptions of additions like the new editor settings. Writerside is a free Jetbrains app or IntelliJ plugin, but the changes you'd need to make are so minor you can probably just edit the raw Markdown/replace existing images.

The documentation updates can also be done in a future PR if you'd prefer to get this merged a bit quicker. I don't really care either way as long as the docs get updated before beta which is still a ways out.

@WindingMotor
Copy link
Contributor Author

I would prefer to use a separate PR for the documentation, thanks!

@mjansen4857
Copy link
Owner

Just checking in to see how things are going with this. I have a branch ready for #690 which touches most files in the project so I want to get this merged before that if you think it'll be ready in the next few days so that I don't cause a million different merge conflicts for you. Again, no rush, just wondering if its worth holding off.

@WindingMotor
Copy link
Contributor Author

It should be ready tonight. I am working on the current conflicts. Thanks for your patience

@WindingMotor
Copy link
Contributor Author

WindingMotor commented Sep 30, 2024

Okay I have merged everything and ran LCOV with 84.4% of the project being covered. I looked into the issue of #723 and could not replicate it.

@mjansen4857
Copy link
Owner

Ok. I'll try to keep a look out for that issue. Thanks for all the work on this.

@JosephTLockwood
Copy link
Contributor

It only appears to happen when "Recent" is the sort by method. I tested again and could not have the issue occur when sorted Alphabetically. It shouldn't be a huge deal, even if it's an issue.

@mjansen4857 mjansen4857 merged commit 39af1a9 into mjansen4857:main Sep 30, 2024
23 checks passed
@WindingMotor
Copy link
Contributor Author

@mjansen4857 Thanks for all your help with this PR, I learned a lot!

@mjansen4857
Copy link
Owner

No problem. Just a heads up I made a couple minor fixes to things I noticed after merging in #776

Main things were some cards were darker than others and the robot details would rotate with the robot.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file GUI Changes to the PathPlanner GUI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants