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

pad_settings: show ds4 battery status in UI; use LED as an indicator #7236

Merged
merged 3 commits into from
Mar 5, 2020

Conversation

Adiost
Copy link
Contributor

@Adiost Adiost commented Jan 12, 2020

  • Add a battery indicator in pad settings dialog for Dualshock 4
  • Add a new LED settings dialog
  • Add an option to use DS4 LED as a battery indicator
  • Fix pad settings dialog window size inconsistency

@Megamouse
Copy link
Contributor

the led already blinks when it's low. why the gradient ?

@Adiost
Copy link
Contributor Author

Adiost commented Jan 12, 2020

LED only blinks at 20% and lower. This is fine, but there's currently no way to tell the battery level anywhere above 20% when in-game. It's intended to be strictly optional, doesn't replace the blinking and allows users to make at least some use of the LED which is otherwise non-functional.

@Megamouse
Copy link
Contributor

Megamouse commented Jan 12, 2020

LED color is meant to be an indicator for players in coop mode. So it is far from non-functional.
And I doubt that you can really interpret the LED as a battery level, unless you drastically change it.

But maybe people like it

@Adiost
Copy link
Contributor Author

Adiost commented Jan 12, 2020

Static LED color is indeed useful to differentiate multiple connected controllers, which is, however, not relevant for single player use cases. The preset gradient currently goes from green at 100% to yellow at 50% to red at 0%. The function as it is right now works as intended, but is disabled (and thus optimized away) as there's no corresponding setting at this moment. If it is to be off by default, it shouldn't interfere with co-op experience, especially since every controller's LED is set to blue and needs to be manually set to a color of choice.

Copy link
Contributor

@Megamouse Megamouse left a comment

Choose a reason for hiding this comment

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

some style changes

rpcs3/Input/ds4_pad_handler.cpp Outdated Show resolved Hide resolved
@Vili1
Copy link

Vili1 commented Jan 12, 2020

NGL this looks dope :v

@Adiost Adiost requested a review from Megamouse January 12, 2020 20:03
@Adiost Adiost changed the title pad_settings: show ds4 battery status in UI [WIP] pad_settings: show ds4 battery status in UI; use LED as an indicator Jan 13, 2020
@Adiost
Copy link
Contributor Author

Adiost commented Jan 14, 2020

Everything that was planned in this PR is now implemented.

The LED battery indicator mode now supports configurable brightness (0-100), properly works with multiple controllers and is no longer limited to in-game. The pre-existing block that blinks the LED is now also user configurable. The LED button/color sample in pad_settings_dialog was removed (which makes the interface more consistent between the handlers), instead the unused space beside the Device type dropdown is now home to a battery level indicator and a button that opens a newly implemented pad_led_settings_dialog.

Additionally, the height of pad_settings_dialog layout and the min height of the description label were altered in order to resolve the issues with handlers that require 4 lines. This change was reverted in 36d3e36 since the new dimensions did not properly fit in a 720p screen. The pad png was scaled down instead, just enough to afford one more line for the handler description label.

A3ewoN

ONrn9N

@Adiost Adiost changed the title [WIP] pad_settings: show ds4 battery status in UI; use LED as an indicator pad_settings: show ds4 battery status in UI; use LED as an indicator Jan 14, 2020
@Adiost Adiost requested a review from Megamouse January 14, 2020 19:35
rpcs3/Input/ds4_pad_handler.cpp Outdated Show resolved Hide resolved
rpcs3/rpcs3qt/pad_led_settings_dialog.cpp Outdated Show resolved Hide resolved
rpcs3/rpcs3qt/pad_settings_dialog.cpp Outdated Show resolved Hide resolved
rpcs3/rpcs3qt/pad_settings_dialog.cpp Outdated Show resolved Hide resolved
@Asinin3
Copy link
Contributor

Asinin3 commented Jan 18, 2020

Works for me, color changed properly. Battery indicator seems to be limited to green which i guess is intended? I had DS4Windows open when i first changed the color, and it changed properly, but then about 5 seconds later went back to the default blue. But, it works fine when ds4windows isn't running.

I didn't get to test the battery light actually changing when battery is low, since I'm using it wired.

@Megamouse
Copy link
Contributor

I have to say that I really don't understand the latest changes

@Adiost
Copy link
Contributor Author

Adiost commented Jan 19, 2020

7bd1843 and 9837e38 deal with the issue of DS4 reporting a battery level after around 12ms after connecting via Bluetooth. It was not apparent previously because the battery level polling was done late enough for the battery level to be available.

@Adiost
Copy link
Contributor Author

Adiost commented Jan 19, 2020

d475644 disables LED battery indication outside of pad_led_settings_dialog because it is not consistent. The feature works as intended in-game, but we're not polling battery level while in settings dialog. Consider this: the user enabled LED battery indication at 50%, the LED becomes yellow. Then the user closes the dialog, keeps RPCS3 open and expects the LED to correctly indicate the battery level, but it will stay yellow until the settings are opened again or the emulation is started.

To make it even more consistent, I'll make it so the LED is reset back to a static color when pad thread is terminated.

@Megamouse
Copy link
Contributor

The explanation is missing as comment in the code.
Also, I don't like the timer. I think you can just add it to get_next_button_press.

@Megamouse
Copy link
Contributor

btw you need to rebase and fix the conflict

@@ -728,6 +751,7 @@ ds4_pad_handler::DS4DataStatus ds4_pad_handler::GetRawData(const std::shared_ptr
return DS4DataStatus::NoNewData;

int battery_offset = offset + DS4_INPUT_REPORT_BATTERY_OFFSET;
device->is_initialized = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

you never set this to false (for example before the second ReadError return above)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's declared as false.

Copy link
Contributor

Choose a reason for hiding this comment

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

devices can disconnect and reconnect

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, thanks!


connect(&m_timer_ds4_battery, &QTimer::timeout, [this]()
{
if(m_handler->get_device_init(m_device_name))
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if(m_handler->get_device_init(m_device_name))
if (m_handler->get_device_init(m_device_name))

@Megamouse
Copy link
Contributor

ups.. wrong button

@Megamouse Megamouse reopened this Jan 19, 2020
@Megamouse
Copy link
Contributor

I think you messed up the formatting in the filters file. ;)

@Adiost
Copy link
Contributor Author

Adiost commented Jan 19, 2020

I think you messed up the formatting in the filters file. ;)

Where? o:

Regarding the timer, I'll play around with it some more, but I have to say that it seems to be the most optimal solution. It's basically a finite cycle in a separate thread with a time limit between ticks. As soon as we get what we want it terminates. Still, I'll experiment some more.

@Megamouse
Copy link
Contributor

what i mean is that you don't have to poll it with a timer if you can just append it to the already existing callback that is called with a timer anyway

@escalade
Copy link

escalade commented Feb 6, 2020

This is very useful, please rebase and get this merged :)

@Adiost
Copy link
Contributor Author

Adiost commented Feb 24, 2020

I'll get back to it in a couple of weeks.

@illusion0001
Copy link
Contributor

This doesn't need more than 5 commits to address something. Needs squishing + rebase

@Nekotekina
Copy link
Member

I can squish during merging.

@Nekotekina
Copy link
Member

No luck with rebase, sorry.

@Adiost
Copy link
Contributor Author

Adiost commented Mar 5, 2020

I'll rebase myself in a week or so. Don't have the time atm.

@Megamouse
Copy link
Contributor

@Adiost the rebase wasn't as trivial due to massive changes in the project structure.
I rebased it on my own, which was quite troublesome, and pushed it to my own ds4 branch.
I also fixed some issues I had with the ui files at the same time.

You can just use my branch and push that code with your name.
Or I can just push that commit into your branch if it doesn't bother you that it has my name on it.

Btw: I didn't change anything in your code apart from the ui stuff (which also didn't change anything visually).
So that means you should still figure out that timer/callback issue I had mentioned.

@Adiost
Copy link
Contributor Author

Adiost commented Mar 5, 2020

@Megamouse Thank you for rebasing and finishing the necessary changes!

@Megamouse Megamouse merged commit e1b4cf1 into RPCS3:master Mar 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants