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

BIG refactor/cleanup, and performance optimization #47

Merged
merged 18 commits into from Jan 13, 2023

Conversation

carleeno
Copy link
Collaborator

@carleeno carleeno commented Jan 1, 2023

Improvements:

  • Simplified data transfer from CAN services to view model by removing intermediate live flow and collector.
  • There is now a LiveData for each signal, and they are only updated when the signal value changes, resulting in far less data being sent
  • DashViewModel provides helpers for observing changes for a single signal, some signals, or all signals
  • Augmented CAN signals automatically calculate and add their values into the carState with real CAN signals, making things like power and smoothAmps accessible anywhere
  • New unit converter class that automatically detects the preferred unit when converting signals
  • Signal names and values split out from Constants to help identify what's what
  • Code refactor, saving about 600 lines in DashFragment moving repeated code into functions and other structure improvements
  • Code cleanup, removed long-unused code and comments (if we really need to get something back we have git)
  • Linted code from hundreds of warnings to just dozens of warnings
  • Bug fixes:
    • RWD/AWD detection fixed
    • Front torque gauge min not updating
    • Minor edge cases around changing to/from splitscreen while in different states such as doors open, charging, etc
    • Fixed light telltales or US, EU, and TW markets (expected telltale behavior was only confirmed for these so far)

This shouldn't functionally or visually change anything, if something is different or unexpected, let me know.

This does include some future code for a TACC icon, but is disabled in this PR, and will be enabled in a future PR once we have an icon to use for it.

Testing

Tested with early access release, reports of noticeable fps improvements totally made this worth it :)

Comparison

Before (dark theme) and after (light theme):
https://photos.app.goo.gl/w3aGsRLAmYkPY7HS9

You can see that before frame times often went above the optimum time (green line), and many more renders were performed than necessary.
After, the renders mostly stay below the optimum frame time, and less renders are performed (which is why the graph looks stuttery, that's a good thing)

@carleeno carleeno marked this pull request as ready for review January 4, 2023 17:28
@carleeno
Copy link
Collaborator Author

carleeno commented Jan 4, 2023

@osunick I think I got the bugs ironed out now, looking for more testers, maybe we can release this to the beta release?

@carleeno
Copy link
Collaborator Author

@osunick I think it's finally ready. We could do another early access to confirm the speed limit sign bug is really fixed if you want, or just send it haha.

When you do merge, I might suggest that you choose the squash and merge option so the main branch doesn't get cluttered with so many small commits from this branch. It makes it easier to read through commit history and revert PRs comprised of lots of smaller commits.

Copy link
Collaborator

@osunick osunick left a comment

Choose a reason for hiding this comment

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

Wow, mega job here.

@osunick osunick merged commit e699c05 into nmullaney:main Jan 13, 2023
@carleeno carleeno deleted the observe-signals branch January 13, 2023 13:25
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.

None yet

3 participants