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

New input gui #1713

Merged
merged 19 commits into from Sep 5, 2019

Conversation

@ergo720
Copy link
Contributor

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

// * 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.

This comment has been minimized.

Copy link
@RadWolfie

RadWolfie Sep 2, 2019

Member

Boston* 😉

We can do this fix later on.

This comment has been minimized.

Copy link
@LukeUsher

LukeUsher Sep 2, 2019

Member

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

This comment has been minimized.

Copy link
@RadWolfie

RadWolfie Sep 2, 2019

Member
@RadWolfie

This comment has been minimized.

Copy link
Member

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.

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

.gitmodules Outdated Show resolved Hide resolved
.gitmodules Outdated Show resolved Hide resolved
@ergo720

This comment has been minimized.

Copy link
Contributor Author

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

This comment has been minimized.

Copy link

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.

@LukeUsher
Copy link
Member

left a comment

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'.

.gitmodules Outdated Show resolved Hide resolved
src/common/Settings.cpp Show resolved Hide resolved
// * 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.

This comment has been minimized.

Copy link
@LukeUsher

LukeUsher Sep 2, 2019

Member

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

src/common/util/CxbxUtil.cpp Outdated Show resolved Hide resolved
@RadWolfie

This comment has been minimized.

Copy link
Member

commented Sep 2, 2019

  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

This comment has been minimized.

Copy link
Member

commented Sep 2, 2019

#1713 (review)

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

This comment has been minimized.

Copy link
Contributor

commented Sep 3, 2019

  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

This comment has been minimized.

Copy link
Member

commented Sep 4, 2019

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

This comment has been minimized.

Copy link
Member

commented Sep 4, 2019

@LukeUsher There is current review change I had requested. Source: #1713 (comment)

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

@RadWolfie

This comment has been minimized.

Copy link
Member

commented Sep 4, 2019

  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

This comment has been minimized.

Copy link
Contributor Author

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

This comment has been minimized.

Copy link
Member

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.

@ergo720

This comment has been minimized.

Copy link
Contributor Author

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.

@RadWolfie
Copy link
Member

left a comment

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.

@RadWolfie RadWolfie merged commit d8b0075 into Cxbx-Reloaded:develop Sep 5, 2019

3 checks passed

Cxbx-Reloaded.Cxbx-Reloaded Build #20190905.9 succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@ergo720 ergo720 deleted the ergo720:input_gui branch Sep 6, 2019

@gandalfthewhite19890404

This comment has been minimized.

Copy link
Contributor

commented Sep 6, 2019

"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
Projects
None yet
5 participants
You can’t perform that action at this time.