-
-
Notifications
You must be signed in to change notification settings - Fork 24
Add hybrid info overlay #284
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
base: dev
Are you sure you want to change the base?
Conversation
Race Element.HUD.Common/Overlays/Driving/HybridInfo/HybridInfoConfiguration.cs
Outdated
Show resolved
Hide resolved
Race Element.HUD.Common/Overlays/Driving/HybridInfo/HybridInfoOverlay.cs
Outdated
Show resolved
Hide resolved
Race Element.HUD.Common/Overlays/Driving/HybridInfo/HybridInfoOverlay.cs
Outdated
Show resolved
Hide resolved
Race Element.HUD.Common/Overlays/Driving/HybridInfo/HybridInfoOverlay.cs
Outdated
Show resolved
Hide resolved
Race Element.HUD.Common/Overlays/Driving/HybridInfo/HybridInfoOverlay.cs
Outdated
Show resolved
Hide resolved
RiddleTime
left a 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.
Added a couple of remarks.
Additionally, can this HUD Scale?
Test it out by adding a constructor to the HybridInfoConfiguration, it will reveal a new hud option that allows you to Scale a HUD.
public HybridInfoConfiguration() => GenericConfiguration.AllowRescale = true;
Race Element.HUD.Common/Overlays/Driving/HybridInfo/HybridInfoOverlay.cs
Outdated
Show resolved
Hide resolved
Race Element.HUD.Common/Overlays/Driving/HybridInfo/HybridInfoOverlay.cs
Show resolved
Hide resolved
|
Feel free to ask for a second review |
|
If you'd like to understand more about the font rendering and how to optimize it: https://github.com/RiddleTime/Race-Element/tree/dev/Race%20Element.HUD.Common/Overlays/Driving/Speedometer |
|
So I updated the code so far The constructor is now only including the _font because its readonly and need to be there. I add the comment of the GetEnergyLevel() suggestion (#284 (comment)) I try to test it today. If not today then on Thursday. |
|
I tested everything and it worked properly. Also implemented you suggestion with GetEnergyLevel() |
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.
Pull request overview
This pull request adds a new Hybrid Info overlay feature specifically designed for iRacing GTP class cars to display the current hybrid energy status. The implementation provides a customizable visual bar with color-coded energy level thresholds and text display.
Key Changes
- New HUD overlay system for displaying hybrid/electric energy state in real-time
- Configurable color thresholds for high/medium/low energy levels
- Visual bar indicator with rounded corners and customizable dimensions
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| Race Element.HUD.Common/Overlays/Driving/HybridInfo/HybridInfoOverlay.cs | Main overlay implementation with rendering logic for energy bar and text display |
| Race Element.HUD.Common/Overlays/Driving/HybridInfo/HybridInfoConfiguration.cs | Configuration class defining customizable parameters for bar appearance and color thresholds |
| Race_Element/Controls/Info/ReleaseNotes.cs | Updated release notes mentioning hybrid car data connector support |
Note: I was unable to create specific inline code comments as the actual PR diff was not accessible in the review context. However, based on examining the repository files, I identified several areas that would benefit from attention:
- Grammar: The overlay description contains "A overlay" which should be "An overlay"
- Code Duplication: Lines 154-155 in HybridInfoOverlay.cs contain duplicate
_energyStringWidthcalculation - Architectural Concern: The
BeforeStopmethod sets Width/Height/RefreshRateHz properties, which conventionally belong in the constructor orBeforeStartmethod based on similar overlays in the codebase - Maintainability: HybridInfoConfiguration.cs imports
RaceElement.HUD.Common.Overlays.Driving.LapDeltaBarbut doesn't use it, and contains tooltips that mention "Delta Bar" (likely copy-pasted from LapDeltaBar) - Unused Code: The
Genericproperty in HybridInfoConfiguration appears to be unused
The implementation otherwise follows established patterns from similar overlays like LapDeltaBar and should function correctly for its intended purpose.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
I don't have got an activate iRacing subscription, would you have got an example(picture or video) that I can use to post on social media and in the app's discord? |
Race Element.HUD.Common/Overlays/Driving/HybridInfo/HybridInfoOverlay.cs
Outdated
Show resolved
Hide resolved
|
Fix the mistake where I put the constructor content in the wrong method I can send you a Screenshot on Thursday. |
|
Also can you remove my old Discord Account from the Discord Server? |
No rush, think I've added you now on discord. |
Yeah, thank you :) |
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.
- Please have a look at your code and look for unused variables or class properties, (see the Configuration class for an example)
- Test the HUD so it can Scale, users will complaign if it can't, in an earlier review I've left a tip on how to enable the scaling option
Pull Request Template
Description
I created a new overlay for the hybrid system energy state. Currently its only working for the iRacing GTP class cars, but its should be possible to convert it also to other sims
Related Issue
Type of Change
Checklist
Additional Notes
In testing