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

Feat/modelspinnermousebutton #5981

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

Conversation

mwerle
Copy link
Contributor

@mwerle mwerle commented Nov 22, 2024

Took over #5594 as @zonkmachine indicated they no longer have the time to complete their PR.

Changes from the last commit:

  • Unify almost all spinning by creating Input::Manager::IsMouseRotatePressed()
    • almost a single place to implement the logic instead of in multiple places throughout the code.
    • the current button combos for rotation are the MMB and Ctrl+LMB
    • The ModelSpinner has its own implementation as it uses ImGui, not Input::Manager
    • TODO: remove the now-obsolete "No middle mouse button" option
    • TODO: add user configuration options for the buttons/modifiers used
  • Add Input::Manager::GetRotateSpeedShiftModifier() to also unify rotation speed modifiers
    • change using the Right-Shift key to the Left-Alt key for the secondary speed and rotation modifiers
  • Fix SystemView deselecting objects when rotation is started
  • Fix ShipView rotating when rotating the ship (via RMB) itself.

Discussion:
Given that the RMB moves the ship itself, would this be the more natural default to spin everything instead of the MMB? That could free up the MMB for panning/moving in the SectorView and SystemView.

zonkmachine and others added 7 commits November 22, 2024 08:21
* 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.
Since the ModelSpinner is using ImGUi, it appears that it blocks the
underlying "Input::Manager" input system, so we have to duplicate the
"IsMouseRotatePressed()" function here using ImGui inputs instead.

TODO: Figure out how we can unify these to avoid duplicating this.
With the new spinning code using Ctrl+LMB as an alternative way to spin
the view, a selected object in the SystemView was deselected whenever
the user tries to spin the view.

By also checking against the Ctrl key when selecting/deselecting objects
in the SystemView this issue is resolved.

TODO: unify the modifier key with Input::Manager::IsMouseRotatePressed()
Mouse rotation is restricted to only when the relevant bindings are
active; that is, either Ctrl+LMB, or MMB. If another mouse-button is
pressed at the same time, it no longer counts as activating rotation.

It still allows for additional modifiers to be pressed, which is required
when used in conjunction with the rotation speed modifiers.

This prevents the issue from having the ShipView rotating while also
firing the laser.

ALSO: unify rotation speed modifiers (in all but the Model Spinner).
ALSO: use the Left-Alt key instead of the Right-Shift key for the
secondary speed modifier; using both Shift-keys is a bit cumbersome.
@bszlrd
Copy link
Contributor

bszlrd commented Nov 22, 2024

Having pan on MMB would be nice

@mwerle
Copy link
Contributor Author

mwerle commented Nov 22, 2024

Discussion: Given that the RMB moves the ship itself, would this be the more natural default to spin everything instead of the MMB? That could free up the MMB for panning/moving in the SectorView and SystemView.

... although that still leaves the question of which combo to use for rotating the camera (not the ship) in the ShipView.. I guess it could stay on MMB..?

<- not a UX designer, I can do the technical implementation but open to suggestion on which actual buttons/modifiers to use for the various tasks (which were summarised in a "Details" pop-up here : #5594 (comment)) ..

@bszlrd
Copy link
Contributor

bszlrd commented Nov 22, 2024

I'd say that can stay on MMB. In Blender at least MMB is always the primary viewport navigation: in 3D view it rotates the view, in 2D windows it pans, and I don't think I ever got confused over that.
Although admittedly, when switching to other software, especially to Krita sometime I do get confused sometimes, and try to use shift+MMB for pan, which spins the view.

@sturnclaw
Copy link
Member

Given that the RMB moves the ship itself, would this be the more natural default to spin everything instead of the MMB? That could free up the MMB for panning/moving in the SectorView and SystemView.

MMB should "generally" be rotate view, with the WASD keys (and/or left stick on controller, equivalent on joystick) serving to move the "map cursors" in Sector/System map view. I find it more helpful to think about the maps as having a discrete cursor (at the center of the view) which is moved rather than moving by panning the view.

Specifically this is because the map cursors in both maps should move on (some sort) of plane, whether that's the equatorial / ecliptic plane or a custom plane for the specific view (i.e. close zoom on planets wraps around the surface via lat/lon).

@@ -846,6 +857,18 @@ void Manager::DispatchEvents()

*/

// Mouse rotation is enabled if:
// MMB is pressed, or Ctrl+LMB is pressed, AND no other mouse buttons are pressed
bool Manager::IsMouseRotatePressed()
Copy link
Member

Choose a reason for hiding this comment

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

I am hesitant to accept this. I do not think (philosophically or practically) that it is the role of the Input::Manager to define the conditions for rotating the view across the entire application. I see Input::Manager as being significantly lower-level and this being the responsibility of a higher layer of the application.

Additionally, note that implementing it at this level provides no ability for the user to rebind which modifier key is used when emulating the middle mouse button (perhaps they'd like to use Alt or the meta keys?)

I wish I had an easy solution I could suggest to accomplish this. The mouse rotation code is one of those things that is common to a number of views, but specific enough in scope it's not really a good fit to throw in Pi, Input::Manager, or View. It's game-specific code, but there's a little bit of overlap with the editor as the editor also uses "game" widgets.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am hesitant to accept this. I do not think (philosophically or practically) that it is the role of the Input::Manager to define the conditions for rotating the view across the entire application. I see Input::Manager as being significantly lower-level and this being the responsibility of a higher layer of the application.

Ok, just seemed like a natural/easy place for this functionality. It can be placed somewhere else instead. I'm very loathe to re-implement it in every place where it's needed though. I very much dislike code duplication and it already chafes me that I have to duplicate between these views and the Ship Spinner..

Additionally, note that implementing it at this level provides no ability for the user to rebind which modifier key is used when emulating the middle mouse button (perhaps they'd like to use Alt or the meta keys?)

Why not? I haven't gotten around to adding options for rebinding yet but I wasn't aware of there being a technical issue with adding such.

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 very loathe to re-implement it in every place where it's needed though.

Same. I do think it should be centralized somewhere (perhaps a static method on some sort of helper namespace?) but just not in the low-level input subsystem.

Why not? I haven't gotten around to adding options for rebinding yet but I wasn't aware of there being a technical issue with adding such.

The "no ability" referred to the as-implemented state. In terms of potential implementations, it's not so much that there is a technical problem with it, but rather that the API becomes significantly messier as the Input::Manager now has to export a specific action binding that all code which wants to use the function needs to ensure is present in the InputFrame stack before calling the method.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants