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

ModelSpinner - use middle mouse button to spin model #5594

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

zonkmachine
Copy link
Member

Use middle mouse button in model spinner.

Fixes #5544

@impaktor impaktor requested a review from Gliese852 July 12, 2023 19:14
@Gliese852
Copy link
Contributor

Also, in my opinion there is a problem with this patch - what to do for people like me - without the middle button (touchpad only) - in other places there is either the ability to twist it with the numpad, or there is a button on the panel for this.

@zonkmachine
Copy link
Member Author

what to do for people like me

Would an option in the settings work? 'No middle button present'

@zonkmachine
Copy link
Member Author

zonkmachine commented Jul 16, 2023

Would an option in the settings work? 'No middle button present'

Tried it out and it seem to work well. There is now a setting option that will set the mousebutton to '0', the old value, instead. I've been wrestling with the compiler for a couple of hours which is reflected in the variable names. I'll clean that up should this function be wanted.

mousebutton

@Gliese852
Copy link
Contributor

@zonkmachine I like the option idea, but I think the option is very global, and it seems that it should somehow affect other places as well - Sector Map, System Map, World View. For example, a person who has a middle button activates this checkbox, but his middle button will still work, which seems odd?

@impaktor
Copy link
Member

The other MMB rotation screens have a "left click and hold"-button [to rotate], no?

@Gliese852
Copy link
Contributor

@impaktor WorldView no.

@zonkmachine
Copy link
Member Author

@zonkmachine I like the option idea, but I think the option is very global, and it seems that it should somehow affect other places as well - Sector Map, System Map, World View. For example, a person who has a middle button activates this checkbox, but his middle button will still work, which seems odd?

Yes, I just picked the ones mentioned in the issue to try it out. I think I got a better grip of the issue now and I've pushed similar changes for the places you mention here. I don't know if there is an instance of rotation that I've missed.

@Gliese852
Copy link
Contributor

I tested the last commit and it seems to work well, I would definitely enable this option for myself. What do you think about it, @impaktor?

@impaktor
Copy link
Member

So if no middle mouse button (option is selected), you rotate with some other mouse button? Or how does this work? What's the trade off?

Also, are there no tooltip for the other control options in the settings menu? (If there are, then this should also have, explaining it changes how to rotate ships and maps)

@zonkmachine
Copy link
Member Author

So if no middle mouse button (option is selected), you rotate with some other mouse button? Or how does this work? What's the trade off?

No trade off. If you don't have a middle mouse button you go back to the old way with the left mouse button. Now also implemented so all spin actions work the same, with either middle mouse button or left mouse button.

Also, are there no tooltip for the other control options in the settings menu?

I haven't seen any for the other options.

@impaktor
Copy link
Member

But left mouse button does other things in some of the screens, right? Like in the maps? (Pardon me being too lazy to read the source, or start the game or test the PR)

@zonkmachine
Copy link
Member Author

zonkmachine commented Jul 17, 2023

But left mouse button does other things in some of the screens, right?

Yes but we already test for hover on. Pioneer knows if you, for instance in the Sector View where you can grab the scene and spin it, or use the button on the right side and grab that and spin it. Left mouse button both times.

Pardon me being too lazy to read the source, or start the game or test the PR

I think that's the way to go. Grabbing something and manipulating it with a mouse is a complex thing to do. It's easy to see what the PR does when you test it and compare to how it is today.

@zonkmachine
Copy link
Member Author

which is reflected in the variable names. I'll clean that up should this function be wanted.

Fixed up variable names and strings. Squashed and ready for review. Tested and works like a charm... :P

@@ -245,6 +248,7 @@ class Input::Manager {

bool joystickEnabled;
bool mouseYInvert;
bool noMiddleMouseButton;
Copy link
Member Author

Choose a reason for hiding this comment

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

I adopted to surrounding code here but shouldn't they be member variables, m_mouseYInvert, m_noMiddleMouseButton... etc?

Copy link
Member

Choose a reason for hiding this comment

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

Technically yes, they're this way for legacy reasons. Let me review the PR further before I make a decision as to whether it's worth converting them.

@Web-eWorks
Copy link
Member

Would an option in the settings work? 'No middle button present'

Tried it out and it seem to work well. There is now a setting option that will set the mousebutton to '0', the old value, instead. I've been wrestling with the compiler for a couple of hours which is reflected in the variable names. I'll clean that up should this function be wanted.

mousebutton

The "best" way to handle this IMO is to have this checkbox toggle on an additional code-path which looks for e.g. alt-LMB / ctrl-LMB and handles that as a middle-mouse press in all screens. The choice of key should most likely be configurable by the user as a binding, and all callsites which use MMB pan/rotate should be updated for this option.

I believe this is how Mac software usually handles pan/rotate when both left+right mouse button have semantic actions already (though without the configurable modifier key). The advantage to this approach is it allows screens with already-heavily overloaded mouse actions (e.g. WorldView) to support interaction without MMB while not sacrificing the ability of the user to quickly perform important actions in combat or flight.

Copy link
Member

@Web-eWorks Web-eWorks left a comment

Choose a reason for hiding this comment

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

There are several regressions with this option enabled:

  • You can no longer left-click on systems in the Sector map to select them.
  • Every time you pan in the System map you deselect the current selected object (changes tree visibility in the outliner and hides the info window).
  • Same with the System atlas map, panning deselects the currently selected object.
  • As mentioned in a separate comment, it becomes impossible to aim while firing in combat.

src/Input.h Outdated Show resolved Hide resolved
Comment on lines 275 to 277
const int mouseButton = (Pi::input->IsMiddleMouseButton() ? SDL_BUTTON_LEFT : SDL_BUTTON_MIDDLE);
bool mouse_down = Pi::input->MouseButtonState(mouseButton);
Copy link
Member

Choose a reason for hiding this comment

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

I'm unwilling to accept this as-is, given that the left-mouse button is overloaded to fire weapons while rotating the ship (in PlayerShipController IIRC). This means that as soon as the player starts firing in combat, their view is going to rotate wildly with every correction of the mouse while aiming.

Copy link
Contributor

Choose a reason for hiding this comment

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

This can probably be a problem if you have a mouse without a middle button or wheel, but in the case of a touchpad, it is pretty convenient to shoot with a spacebar.

Copy link
Member

@Web-eWorks Web-eWorks Aug 5, 2023

Choose a reason for hiding this comment

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

Certainly, but the expectation for most people playing on desktop is they fire with a mouse button click. Spacebar for most people is mentally associated with "go up" rather than "shoot", especially in 6-DOF style games.

EDIT: either way, the feedback here would not affect the ability to fire with spacebar, but rather mentioning that the current version of the PR accidentally removes the ability to shoot with a mouse.

@zonkmachine
Copy link
Member Author

Tried it out and it seem to work well. There is now a setting option that will set the mousebutton to '0', the old value, instead. I've been wrestling with the compiler for a couple of hours which is reflected in the variable names. I'll clean that up should this function be wanted.
mousebutton

The "best" way to handle this IMO is to have this checkbox toggle on an additional code-path which looks for e.g. alt-LMB / ctrl-LMB and handles that as a middle-mouse press in all screens. The choice of key should most likely be configurable by the user as a binding, and all callsites which use MMB pan/rotate should be updated for this option.

I believe this is how Mac software usually handles pan/rotate when both left+right mouse button have semantic actions already (though without the configurable modifier key). The advantage to this approach is it allows screens with already-heavily overloaded mouse actions (e.g. WorldView) to support interaction without MMB while not sacrificing the ability of the user to quickly perform important actions in combat or flight.

I was apparently way less done here than I thought. I'll look into using the alt/ctrl key workaround instead. In any case people with no middle button need a workaround.

But left mouse button does other things in some of the screens, right?

Now I understand your question.

Thank you all for your reviews.

@zonkmachine zonkmachine marked this pull request as draft July 30, 2023 11:36
@impaktor
Copy link
Member

impaktor commented Jan 9, 2024

I think the aim of this PR is great: to unify MMB for spinning, and an checkbox option for those without MMB, to then use modifier-key + LMB, for instance. But as noted in the three last posts above, there's more to do before this PR is ready for review.

Hope @zonkmachine finds time for this again at some point & rebase to prevent bitrot.

@zonkmachine
Copy link
Member Author

Hope @zonkmachine finds time for this again at some point & rebase to prevent bitrot.

Sorry for the late reply. I'm busy with real world stuff and won't have time to look into this anytime soon. I unfortunately failed to rebase it on current master. If someone want's to pick up this PR they are most welcome.

* Make grab/spin action consistent and default to middle mouse button.
* Option to use left mouse button in cases where no middle mouse button
  is available such as on a laptop.
@zonkmachine
Copy link
Member Author

Hope @zonkmachine finds time for this again at some point & rebase to prevent bitrot.

Done. I'm looking into the feedback from the review.

@zonkmachine
Copy link
Member Author

* As mentioned in a separate comment, it becomes impossible to aim while firing in combat.

Maybe activate this only when landed/docked?

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.

Wrong button for rotating in shipspinner
4 participants