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

Don't transform relative values when using MOUSE_MODE_CAPTURED #57264

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

madmiraal
Copy link
Contributor

When MOUSE_MODE_CAPTURED is used, InputEventMouseMotion's relative property provides raw mouse motion data. Therefore, this data should not be transformed by the current Viewport transform.

This PR ensures that InputEventMouseMotion's relative data is not transformed when using MOUSE_MODE_CAPTURED i.e. when it contains raw mouse motion data.

Fixes #49734
Fixes #54818

Copy link
Member

@bruvzg bruvzg left a comment

Choose a reason for hiding this comment

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

Seems reasonable to me. Probably need to be mentioned in the InputEventMouseMotion docs to avoid any confusion.

@reduz
Copy link
Member

reduz commented Jan 26, 2023

We've discussed this in the Input PR meeting and basically the result of the discussion is:

  • There are cases where you want it scaled even if captured (example, dragging a slider with the control scaled).
  • There are cases where you don't want it scaled even if captured (example, rotating a cube on the viewport by dragging).

And in both you need the GUI context, so you can't use the raw input.

So we think the best solution would be to add a global_relative property (similar to global_position) that is unscaled, and you use the one that best works for your needs.

@akien-mga akien-mga modified the milestones: 4.0, 4.x Feb 17, 2023
@Calinou
Copy link
Member

Calinou commented Aug 23, 2023

I tend to agree with reduz's opinion here. There isn't a single good behavior for relative mouse movement that covers all use cases. This also allows implementing this feature/fix without having to break compatibility in any way, now that 4.0 is released.

That said, global_relative may be a bit confusing (it sounds like it has to do with coordinate systems), so I'd suggest unscaled_relative instead.

@crashfort
Copy link

crashfort commented Sep 22, 2023

This is still an issue, and a clear simple solution has been given over 19 months ago. You have to apply a hacky workaround for this in client code in order to "unscale" it (which is the value that you will always want when using raw input). When you interact with UI stuff you are not using raw input because raw input does not contain the mouse pointer ballistics. So I don't understand what situation you mentioned in your meeting.

@danielborgmann
Copy link

I'm not sure I have fully wrapped my head around every possible use case in a 2D / UI scenario, but at least in a 3D game scenario you never want to scale this value. It is not a minor issue either, if careless developers default to the scaling behaviour for mouse input, it will be perceived as very broken by players and this could easily tarnish the reputation of the engine if it becomes a common issue for games made with Godot.

Even if there are cases where the scaled behaviour is desired, I believe that defaulting to the scaling behaviour violates the Godot philosophy of making the common case easy and the rare case possible. Adding a work-around to apply scaling is a lot more intuitive than to "undo" the scaling, likewise it seems strange to me to have a default property that scales and a more specialized property that doesn't.

If you absolutely must keep the behaviour of the current property, then I would suggest to simply call the new unscaled property "delta" or "raw_delta" and make it clear in the comments for this and the "relative" property what the differences are and when you want to use which.

All that aside I do agree that "unscaled_relative" would be a lot less potentially confusing than "global_relative", but neither is really good IMO for something that is the expected default behaviour.

@Calinou
Copy link
Member

Calinou commented Jan 25, 2024

I've tried to add relative_unscaled and velocity_unscaled properties to InputEventMouseMotion and InputEventScreenDrag: https://github.com/Calinou/godot/tree/inputevent-add-relative-velocity-unscaled

However, it doesn't seem to work for some reason. The values are always 0 even when they shouldn't be. This occurs both with input accumulation enabled and disabled. What's strange is that if I print the value in the C++ implementations of set_relative_unscaled() and get_relative_unscaled(), I can see the value being set correctly in the member variable.

Testing project for the above branch: test_unscaled_relative_input.zip

simplescreenrecorder-2024-01-26_00.00.00.mp4

@gelvinp
Copy link
Contributor

gelvinp commented Jan 26, 2024

@Calinou I believe I see the reason why you are seeing zeros.

Collapsed to avoid noise

Viewport::push_input with p_local_coords = false (the default) calls make_input_local which calls xformed_by on the InputEventMouseMotion.

xformed_by creates a new InputEventMouseMotion instance. You left a comment saying that they are intentionally not transformed, but they are never set to anything, so when push_input replaces the InputEvent with the one returned by xformed_by, it has no unscaled values.

If I add the lines

	mm->set_relative_unscaled(get_relative_unscaled());
	mm->set_velocity_unscaled(get_velocity_unscaled());

to xformed_by, then I get values in the editor (they start out the same, but when I stretch the window they have different values):

image

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