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

mouse_position is not consistent in one _process() call in Godot 4 #89109

Open
toxicvgl opened this issue Mar 3, 2024 · 10 comments
Open

mouse_position is not consistent in one _process() call in Godot 4 #89109

toxicvgl opened this issue Mar 3, 2024 · 10 comments

Comments

@toxicvgl
Copy link

toxicvgl commented Mar 3, 2024

Tested versions

Reproducible in: v4.3.dev4 [df78c06], v4.2.1.stable [b09f793] - Not reproducible in: v3.5.3.stable [6c81413]

System information

Godot v4.3.dev4.mono - Windows 10.0.22631 - Vulkan (Forward+) - dedicated NVIDIA GeForce RTX 4050 Laptop GPU (NVIDIA; 31.0.15.4626) - AMD Ryzen 5 7640HS w/ Radeon 760M Graphics (12 Threads)

Issue description

get_local_mouse_postion() or get_global_mouse_position() returns different coordinates call by call in one _process().

[EDIT] Turns out (thanks to comments below) this is due to a ''feature'' of DisplayServer.mouse_get_position(), which returns the mouse position out of the Godot frame system. Discussion on design may be needed.

Steps to reproduce

  1. Create a new project and a scene with any root node
  2. Add a CanvasLayer as a child
  3. Add a ColorRect as a child node of the layer, use PRESET_FULL_RECT anchors
  4. Attach the script below to the ColorRect
  5. Run the scene and move mouse around quickly
  6. In my laptop get_local_mouse_position() and get_global_mouse_position() are not consistent
extends ColorRect

func _process(_delta: float) -> void:
	var global_position_first = get_global_mouse_position()
	var global_position_second = get_global_mouse_position()
	var position_first = get_local_mouse_position()
	var position_second = get_local_mouse_position()
	
	if global_position_first != global_position_second:
		print(global_position_first, global_position_second, " Why global position D:")
	if position_first != position_second:
		print(position_first, position_second, " Why local position D:")

Minimal reproduction project (MRP)

N/A

@toxicvgl toxicvgl changed the title get_mouse_position is not consistent in one _process() call in Godot 4 mouse_position is not consistent in one _process() call in Godot 4 Mar 3, 2024
@KoBeWi
Copy link
Member

KoBeWi commented Mar 3, 2024

The error seems to be random (one printed every few seconds of moving):

(571, 287)(572, 287) Why global position D:
(302, 232)(245, 244) Why global position D:
(523, 366)(519, 365) Why global position D:
(585, 310)(601, 302) Why global position D:
(319, 295)(318, 295) Why global position D:
(587, 418)(587, 417) Why global position D:

Also every time it's 1 px difference at most. Maybe there is some rounding issue involved.

@kleonc
Copy link
Member

kleonc commented Mar 3, 2024

Steps to reproduce

1. Create a new project and a scene with any root node
2. Add a **CanvasLayer** as a child
3. Add a **ColorRect** as a child node of the layer, use PRESET_FULL_RECT anchors
4. Attach the script below to the ColorRect
5. Run the scene and move mouse around quickly
6. In my laptop get_local_mouse_position() and get_global_mouse_position() are not consistent

CanvasLayer / ColorRect are unrelated here, steps 2/3 are not needed, it happens even for a plain Node2D.

extends Node2D

func _process(_delta: float) -> void:
	var global_position_first = get_global_mouse_position()
	var global_position_second = get_global_mouse_position()
	var position_first = get_local_mouse_position()
	var position_second = get_local_mouse_position()
	
	if global_position_first != global_position_second:
		print(global_position_first, global_position_second, " Why global position D:")
	if position_first != position_second:
		print(position_first, position_second, " Why local position D:")

(302, 232)(245, 244) Why global position D:
(523, 366)(519, 365) Why global position D:
(585, 310)(601, 302) Why global position D:

Also every time it's 1 px difference at most.

It's not, relook at your own example output. 🙃


I don't think this behavior is a bug. AFAICT all transforms involved into calculations are not being changed when this happens, calculations are all the same. The different results are caused simply by the OS / DisplayServer returning different mouse coordinates for the different CanvasItem.get_{local,global}_mouse_position calls. I'd say that's expectable given they're fetched at different timestamps.

It's not said anywhere that CanvasItem.get_{local,global}_mouse_position returns some per-process-frame cached value.

Specifically, for the root Viewport get_mouse_position ends up returning this:

return xform.affine_inverse().xform(DisplayServer::get_singleton()->mouse_get_position());

As mentioned, xform remains the same between the calls in question, it's the return value of DisplayServer::get_singleton()->mouse_get_position() which changes between these calls.

And here's e.g. its implementation for Windows:

Point2i DisplayServerWindows::mouse_get_position() const {
POINT p;
GetCursorPos(&p);
return Point2i(p.x, p.y) - _get_screens_origin();
}

@Sauermann
Copy link
Contributor

Can confirm the behavior on Linux X11 Xfce.

The behavior can be replicated even simpler by going directly to the DisplayServer from a Node .... No Node2D is needed.

extends Node


func _process(_delta: float) -> void:
	var f1 = DisplayServer.mouse_get_position()
	var f2 = DisplayServer.mouse_get_position()
	if f1 != f2:
		print ("DS inconsistent: ", f1, f2)

Is it possible, that this can't be mitigated in Godot, because the DisplayServer queries the current mouse position from the OS and the mouse has traveled between the two calls?

@toxicvgl
Copy link
Author

toxicvgl commented Mar 4, 2024

Thank you all for your insights, I am learning.

Indeed in my usecase, I can call var godot_frame_mouse_position = get_global_mouse_position() once in _process() in the scene root node, and pass the data around. However,

  • in the current behavior different nodes will get different coordinates in one Godot frame. Doc may need more information if this is not considered as a bug.
  • this might be causing another issue on _gui_input() for example, since DisplayServer.mouse_get_position is changing outside the Godot frame cycle.

The second point is actually why I noticed this behavior, but maybe unrelated (sorry if so). I was coding a simple drag & drop system on a Panel node. For that, I set the center position of the Panel to the mouse position while _gui_input(). In theory my cursor should not go out the Panel so easily (I know I should use _process() for safety), but practically it happened so often.

@thygrrr
Copy link
Contributor

thygrrr commented Mar 9, 2024

Got the same. Obvious bug on Windows 11 pro. (not calling this a bug is IMHO inacceptable)

This bug affects me because to compensate for Camera2D's zoom behaviour (to zoom into the position of the mouse), I need to get the global mouse position twice in one _process. Because I zoom smoothly, I need to get both every frame to move the camera accordingly. (alternate solution sought, not sure the limited Camera2D functionality can help with that - I'll try getting the screen position once and doing my transforms with that)

Repro:

extends Node2D

func _process(delta: float) -> void:
	var pos1 = get_global_mouse_position()
	var pos2 = get_global_mouse_position()
	print(pos1-pos2)
	if (pos1 != pos2):
		print("unexpected deviation!")

mouse_unstable_repro.zip

Repro project: run, wiggle the mouse, look at console.

@thygrrr
Copy link
Contributor

thygrrr commented Mar 10, 2024

There is a workaround for my scenario in 2D:

extends Camera2D

@export var maxZoom : float = 5.0
@export var minZoom : float = 0.1
@export var stepRatio : float = 0.9

@export var smoothing : float = 0.9
@export var referenceFPS : float = 120.0

@onready var zoom_goal := zoom
@onready var position_goal := position

var last_mouse : Vector2
var zoom_mouse : Vector2

func zoom_and_pan() -> void:
	var current_mouse := get_local_mouse_position()
	if Input.is_action_pressed("look>pan"):
		position_goal += (last_mouse - current_mouse)

	if Input.is_action_just_pressed("look>zoom+"):
		zoom_goal *= (1.0 / stepRatio)
		zoom_mouse = get_viewport().get_mouse_position()
		zoom_mouse -= get_viewport_rect().size * 0.5

	if Input.is_action_just_pressed("look>zoom-"):
		zoom_goal *= stepRatio
		zoom_mouse = get_viewport().get_mouse_position()
		zoom_mouse -= get_viewport_rect().size * 0.5

	zoom_goal = zoom_goal.clamp(minZoom * Vector2.ONE, maxZoom * Vector2.ONE)
	last_mouse = current_mouse



func _process(delta: float) -> void:
	zoom_and_pan()

	var k := pow(smoothing, referenceFPS * delta)

	var mouse_pre_zoom := to_local(get_canvas_transform().affine_inverse().basis_xform(zoom_mouse))
	zoom = zoom * k + (1.0-k) * zoom_goal
	var mouse_post_zoom := to_local(get_canvas_transform().affine_inverse().basis_xform(zoom_mouse))

	var zoom_position_offset := (mouse_pre_zoom - mouse_post_zoom)

	position_goal += zoom_position_offset
	position = position * k + (1.0-k) * position_goal + zoom_position_offset

However, in 3D it feels better if the zoom follows the mouse cursor even if the mouse moves before the zoom action is done.

@kleonc
Copy link
Member

kleonc commented Mar 10, 2024

While the current behavior "does make sense", after rethinking, I do agree that it indeed can be unexpected, especially given it used to work differently.

Specifically, it's working like that since v4.0.rc1.official [8843d9a], the change was made in #71768.

So I agree we should probably restore the previous behavior which was using a cached mouse position. In case user would want more granularity then they could always manually fetch the mouse position from the DisplayServer.


@Sauermann I assume this new behavior is an unintentional byproduct of the change in #71768?

If I read it right, the problem is that subviewports don't cache the mouse position, so if using cached mouse position would be restored for the main viewport then it would make it work inconsistenly compared to subviewports?

Is caching mouse position for subvieports feasible? Does it hurt performance or is there some other problem?


Alternatively, instead of just restoring the previous behavior, maybe Viewport::get_mouse_position, CanvasItem::get_{local,global}_mouse_position and alike could take in a bool parameter like use_cached_mouse_position defaulted to true? This way it would by default work like it used to in 3.x and it would still allow more granular fetching when desired (without the need to manually transform into proper coord space like when using DisplayServer::get_mouse_position()).


However, in 3D it feels better if the zoom follows the mouse cursor even if the mouse moves before the zoom action is done.

@thygrrr To make it clear, you're saying that the current behavior (not using cached mouse position) makes it feel better, right?
If yes, then this seems like an argument for making it tweakable in Viewport::get_mouse_position etc.

@Sauermann
Copy link
Contributor

Sauermann commented Mar 10, 2024

@kleonc

I do agree that it indeed can be unexpected, especially given it used to work differently.

It used to work differently in 3.x. Since 4.0-stable the behavior is the same.

So I agree we should probably restore the previous behavior which was using a cached mouse position. In case user would want more granularity then they could always manually fetch the mouse position from the DisplayServer.

Simply reverting #71768 would probably not be a good idea, because it would reintroduce situations, where the mouse-position would be completely wrong. So we need to be wary, not to introduce these fixed bugs.

I assume this new behavior is an unintentional byproduct of the change in #71768?

The new behavior was intended, but the effect it shows now, came unexpected.

Is caching mouse position for subvieports feasible? Does it hurt performance or is there some other problem?

Using per-viewport cached mouse position will become problematic in the case of multiple SubViewports:
When the mouse is not over a SubViewport, its cached position will not be updated.
So there would be cases, where queries to the cached mouse position could return the position, where the mouse was several frames ago.

Alternatively, instead of just restoring the previous behavior, maybe Viewport::get_mouse_position, CanvasItem::get_{local,global}_mouse_position and alike could take in a bool parameter like use_cached_mouse_position defaulted to true?

Am I correct with my understanding, that such a change is considered a breaking change?


I thought about this and had a few ideas about how to approach the situation.

One idea is to store in addition to the cached mouse position the frame-number, when the mouse position was updated.
Accessing the mouse position cache would require to compare the cached frame-number with the current frame number and automatically update it, when the cache comes from a previous frame.

A second idea is to introduce the cache (position + frame number) not in every viewport, but only in Window nodes, that represent OS-native windows. That approach allows the caching to be moved from Viewport to the more peripheral class Window.

A third variant would be to introduce in the DisplayServer class additionally DisplayServer.mouse_get_position_in_current_frame, which handles caching and make necessary adjustments in the Viewport class.


Another complexity, that likely needs to be considered is the order of steps in the MainLoop. Within a single frame the order of execution is:

  1. _physics_process
  2. _process
  3. _input

Input events are handled at the end of a single frame. So a new position is delivered in _input, that (while the mouse is moving) greatly differs from the one in _physics_process and _process.

extends Node


func _physics_process(delta: float) -> void:
	pm = get_global_mouse_position()
	prints ("\nPPPP", pm, get_tree().get_frame())


func _process(delta: float) -> void:
	var m = get_global_mouse_position()
	prints ("SSSS", m, get_tree().get_frame())
	if m != pm:
		prints ("difference", pm, m , get_tree().get_frame())


func _input(event: InputEvent) -> void:
	if event is InputEventMouseMotion:
		prints ("MMMM", get_global_mouse_position(), get_tree().get_frame())

Example output:

PPPP (1101, 101) 279
SSSS (1101, 101) 279
MMMM (1088, 144) 279

@kleonc
Copy link
Member

kleonc commented Mar 10, 2024

Alternatively, instead of just restoring the previous behavior, maybe Viewport::get_mouse_position, CanvasItem::get_{local,global}_mouse_position and alike could take in a bool parameter like use_cached_mouse_position defaulted to true?

Am I correct with my understanding, that such a change is considered a breaking change?

Adding a parameter with a default value should be fine as long as it preserves the pre-addition behavior. So false should be fine compatibility-wise. Whether setting the default to true would be breaking I'd say depends on whether the current behavior would be classified as a bug. If it's considered a bug then fixing/changing this would be justified (and hence defaulting to true as well), even if doing so potentially breaks some projects relying on this (fixing bugs often breaks the previous incorrect behavior).

AFAICT it's not documented in detail how exactly the methods in question work (whether they use some cache or always fetch from the OS etc.) so how the issue will be classified seems like an arbitrary decision to me. And decision either way makes sense to me.

Anyway, I kinda dislike this additional parameter I've suggested, as it would a little pollute/complicate the API of such simple functions. Customization of the behavior seems nice, but I'm not convinced it's needed (or maybe not at these functions' level?).


Regarding your ideas for the solution they all seem sensible, I'm not sure what's the best / preferred way to go about it.
The subject seems fundamental so I'd say it needs some input / decision from the core maintainers either way.

@thygrrr
Copy link
Contributor

thygrrr commented Mar 12, 2024

@thygrrr To make it clear, you're saying that the current behavior (not using cached mouse position) makes it feel better, right? If yes, then this seems like an argument for making it tweakable in Viewport::get_mouse_position etc.

No, I meant "it's required to call this each frame to make it feel good", which then triggers the issue with the jitter.

But there might be a way to cache the value once anyway, but it would be a different value (the one from my "workaround"; not the actual result of get_global_mouse_position().

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

No branches or pull requests

5 participants