Skip to content

New input gui#1713

Merged
RadWolfie merged 19 commits into
Cxbx-Reloaded:developfrom
ergo720:input_gui
Sep 5, 2019
Merged

New input gui#1713
RadWolfie merged 19 commits into
Cxbx-Reloaded:developfrom
ergo720:input_gui

Conversation

@ergo720
Copy link
Copy Markdown
Member

@ergo720 ergo720 commented Sep 1, 2019

This implements a new input GUI which completely replaces the old one. It uses Sdl input to handle all non XInput controllers (which are instead handled by XInput) and the keyboard is handled by DInput. The appropriate API is automatically selected so you don’t have to select it now like you had to with the old GUI. Code to support the mouse is present but disabled as it has an unresolved issue which leads to weird movements in games and needs more investigation. Rumble functionality has been implemented so your controllers can also vibrate now (tested with XInput, Sdl needs testing). I also made some changes to the Xbox XInput functions so you will want to retest many games to see if I haven’t accidentally broken something. Please note that, according to here JayFoxRox/xqemu#4 (comment) , not all games accept input from ports other than 1. So, if a game doesn’t accept input from a port, try to connect it to port 1 and if it works, than it’s not a regression/bug.

Edit: Ready for merge

Comment thread src/common/input/DInputKeyboardMouse.cpp Outdated
// * You should have recieved a copy of the GNU General Public License
// * along with this program; see the file COPYING.
// * If not, write to the Free Software Foundation, Inc.,
// * 59 Temple Place - Suite 330, Bostom, MA 02111-1307, USA.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Boston* 😉

We can do this fix later on.

Copy link
Copy Markdown
Member

@LukeUsher LukeUsher Sep 2, 2019

Choose a reason for hiding this comment

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

Didn't we already fix the Bostom=>Boston globally? If so, then it should be fixed here rather than in a later PR.

Copy link
Copy Markdown
Member

@RadWolfie RadWolfie Sep 2, 2019

Choose a reason for hiding this comment

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

@RadWolfie
Copy link
Copy Markdown
Member

RadWolfie commented Sep 2, 2019

Did we discussed about missing save/cancel buttons before? Ah found the mention about missing buttons. Well, it's also missing in this pull request.
https://github.com/Cxbx-Reloaded/Cxbx-Reloaded/pull/1558#issuecomment-472262510

If we're going to merge this without the missing, apply/cancel, buttons, then we'll need to open a new issue for it. Since I had not heard of general UI dialog always save the change.

Ignore this, native close button doesn't perform the save.

Comment thread .gitmodules Outdated
Comment thread .gitmodules Outdated
@ergo720
Copy link
Copy Markdown
Member Author

ergo720 commented Sep 2, 2019

Did we discussed about missing save/cancel buttons before? Ah found the mention about missing buttons. Well, it's also missing in this pull request.
#1558 (comment)
If we're going to merge this without the missing, apply/cancel, buttons, then we'll need to open a new issue for it. Since I had not heard of general UI dialog always save the change.

  1. I could, but I don't see the utility of adding a popup to the Clear button? I feel it would instead be an hindrance if every time the button is pressed, the popup shows up.
  2. Ok, but with one change: the user must have provided a name for the profile. This is because empty profile names are not allowed and are used during profile searches.
  3. Save/Apply/Cancel buttons weren't asked in that comment. Anyway, there's already a Save button in the GUI which does what you want, namely, saving your configuration, so adding another one is just confusing. Apply also feels unnecessary since the button is supposed to save your configuration without closing the window, which is what Save already does. Not sure about the Cancel button at the moment.

@jeltaqq
Copy link
Copy Markdown

jeltaqq commented Sep 2, 2019

Tested with Sdl
Working
Gunvalkyrie
Quantum Redshift
Not working (regression)
Tenerezza

I'm fond of the stick and D-Pad settings "Up down left right" like old GUI. In addition, the same order as the trigger can be set intuitively.

Copy link
Copy Markdown
Member

@LukeUsher LukeUsher left a comment

Choose a reason for hiding this comment

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

Generally works well in the games I tested. Configuration is, much better than our current input manager/configuration.

One issue I had that is that the purpose of the 'profile' input is a little unclear: Do you think we could provide one preset profile so that users can configure and save their settings without having to come up with a profile name?

The purpose of the 'modifier' input is also unclear.

Another issue is that I was able to lose my configuration by doing the following:

  1. Create a Profile, named 'Test'
  2. Configure inputs as you desire
  3. Save
  4. Run the emulator, test config, it works
  5. Open configuration editor, delete your profile, the configured inputs are still shown in the input fields
  6. On closing the dialog, input was still working as configured
  7. On re-opening the dialog, all input mappings were lost (due to not having a profile)

Is there any way we can avoid this scenario? Perhaps we can add some text to the dialog to explain how it should be used, or warn the user if they have configured inputs that do not belong/have not been save to a profile?

Additionally, it seems awkward having the order defined as 'Up, Down, Right, Left' rather than 'Up, Down, Left, Right'. probably just a personal preference, but it doesn't seem 'right'.

Comment thread .gitmodules Outdated
Comment thread src/common/Settings.cpp
// * You should have recieved a copy of the GNU General Public License
// * along with this program; see the file COPYING.
// * If not, write to the Free Software Foundation, Inc.,
// * 59 Temple Place - Suite 330, Bostom, MA 02111-1307, USA.
Copy link
Copy Markdown
Member

@LukeUsher LukeUsher Sep 2, 2019

Choose a reason for hiding this comment

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

Didn't we already fix the Bostom=>Boston globally? If so, then it should be fixed here rather than in a later PR.

Comment thread src/common/util/CxbxUtil.cpp Outdated
@RadWolfie
Copy link
Copy Markdown
Member

  1. I could, but I don't see the utility of adding a popup to the Clear button? I feel it would instead be an hindrance if every time the button is pressed, the popup shows up.

Supposedly you have keyboard input in progress to setup then you had accidentally clear it. You'll have to manually re-input 25 fields. You can check if any inputs had occur then show the popup.

  1. Ok, but with one change: the user must have provided a name for the profile. This is because empty profile names are not allowed and are used during profile searches.

👍 Yeah, users will be confuse why there's a profile name field. Then assume it's not necessary after setting up their inputs.

  1. Save/Apply/Cancel buttons weren't asked in that comment. Anyway, there's already a Save button in the GUI which does what you want, namely, saving your configuration, so adding another one is just confusing. Apply also feels unnecessary since the button is supposed to save your configuration without closing the window, which is what Save already does. Not sure about the Cancel button at the moment.

After 2nd review, looks like the native close button just close it. Perhaps add a close button next to clear button?

@RadWolfie
Copy link
Copy Markdown
Member

https://github.com/Cxbx-Reloaded/Cxbx-Reloaded/pull/1713#pullrequestreview-282515069

To follow up @LukeUsher's comment. Once saved then close/re-open, the profile name is blank which should be showing the profile name.

@gandalfthewhite19890404
Copy link
Copy Markdown
Contributor

  1. What is "XInput Default" labeled button?
  2. Settings are not saving until you create profile or choose (if you need reconfigure it later)
  3. Modifier - what is it, thumb (analogue stick button)?

@LukeUsher
Copy link
Copy Markdown
Member

I think it's still unclear that the user must type a profile name, and cannot simply configure-and-go.
Other than that, this is good and I think it can be merged after this is solved, perhaps by setting a default name if no profiles or present, or even just by adding some instruction text to the dialog.

@RadWolfie
Copy link
Copy Markdown
Member

@LukeUsher There is current review change I had requested. Source: https://github.com/Cxbx-Reloaded/Cxbx-Reloaded/pull/1713#discussion_r319782952

Beside that, I had not re-test the recent change at the moment. I will test it out very soon.

@RadWolfie
Copy link
Copy Markdown
Member

  1. What is "XInput Default" labeled button?

Hmm, supposedly in the future. We may will have default setup for keyboard/mouse. So, it's better to use "Default" button and disable the button if using keyboard/mouse is selected.

  1. Settings are not saving until you create profile or choose (if you need reconfigure it later)

Cause by inexperience users or forgot to save new change(s). We'll have to expect multiple false positive report for this. Plus complaints too.

@ergo720
Copy link
Copy Markdown
Member Author

ergo720 commented Sep 4, 2019

I know that there are still issues and the work is still incomplete. I'm just pushing commits to allow early testing and discover eventual bugs sooner rather than later.

@RadWolfie
Copy link
Copy Markdown
Member

RadWolfie commented Sep 5, 2019

As of commit a001e46, the GUI look complete to me even the recent GUI code changes too.

UPDATE: Okay I found one tiny issue, the rumble test doesn't stop until input UI are completely close.

Comment thread src/common/input/InputWindow.cpp
@ergo720
Copy link
Copy Markdown
Member Author

ergo720 commented Sep 5, 2019

I suppose that happens with SDL controllers? Because with my XInput controller it rumbles for a fixed amount of time and stops normally, independently of the input GUI. I don't have SDL controllers which support vibration so I can't see it myself.

Copy link
Copy Markdown
Member

@RadWolfie RadWolfie left a comment

Choose a reason for hiding this comment

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

Tested with

  • Lego Star Wars 1 & 2
  • Just Cause
  • Star Wars of the Old Republic 1 & 2
  • Crazy Taxi 3 - High Roller

Titles above did not regress.

@gandalfthewhite19890404
Copy link
Copy Markdown
Contributor

"XInput Default" labeled button is present even with dinput/keyboard

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.

5 participants