mouse.x and mouse.y can be outside the game window in Ubuntu 16.04

[ericoporto]

Describe the bug
I was just testing AGS current master in Ubuntu 16.04, and my game was crashing for null pointer in a point, when using Point* rp = Screen.ScreenToRoomPoint(mouse.x, mouse.y); in AGS Script.

Adding a breakpoint in the line below, led me to figuring out `mouse.x' was negative, when I moved it at left at the screen.

ags/Engine/ac/screen.cpp

Line 158 in 44e304f

return nullptr;

AGS Version
Current master (44e304f).

Expected behavior
I expected mouse.x and mouse.y to always be restricted to the game window bounds.

Now, I am not 100% this has always been the expected behavior or if this is the behavior everywhere, but I was under the impression it was. It's possible also that I am hitting some upstream bug in SDL2 instead, since this is a relatively old OS, and it's currently unsupported.

If it's the expected behavior, than the fix can be something like

    scmouse.x = Math::Clamp(game_to_data_coord(mousex), 0, gfxDriver->GetNativeSize().Width);
    scmouse.y = Math::Clamp(game_to_data_coord(mousey), 0, gfxDriver->GetNativeSize().Height);

in

ags/Engine/ac/mouse.cpp

Lines 352 to 355 in 44e304f

	void update_script_mouse_coords() {
	scmouse.x = game_to_data_coord(mousex);
	scmouse.y = game_to_data_coord(mousey);
	}

Additional context

In the manual for Screen.ScreenToRoomPoint, it mentions

The restrictToViewport parameter tells what to do if no viewport is found under the cursor. If it's false, then in such case the conversion will be done through the primary viewport. If it's true, then this function returns null if no viewport is there. The default is false.

So I expected it to never be null, unless that parameter was true, but I did not consider the case the coordinates were outside of screen bounds.

Desktop:

• OS: Ubuntu
• Version 16.04

[ivan-mogilko]

Clamping the mouse is one thing, but Screen.ScreenToRoomPoint() with restrictToViewport = false should not return null regardless of coordinates. Is it possible to double check that it returns null if you pass e.g. -1, -1, or other out-of-bounds values?

[ericoporto]

I tried the following in Windows

function room_AfterFadeIn()
{
  int mx = -100; 
  int my = 1;
  Point* p = Screen.ScreenToRoomPoint(mx, my);
  if(p == null) {
    Display("Screen.ScreenToRoomPoint(%d, %d) == null", mx, my);
  } else {
   Display("Screen.ScreenToRoomPoint(%d, %d) == Point(%d, %d)", mx, my, p.x, p.y);
  }
}

And it worked as expected, no null.

I then tried.

void repeatedly_execute_always()
{
  int mx = -100; 
  int my = 1;
  Point* p = Screen.ScreenToRoomPoint(mx, my);
  if(p == null) {
    Display("Screen.ScreenToRoomPoint(%d, %d) == null", mx, my);
  }
}

And it never displayed... In the game I was playing around in Ubuntu, I had Multitasking mode turned on. I tried to turn it on, in Windows, but it did not affect anything. I will test things again in Linux later.

Here's above test project so I can try in the other PC later: TestScreenToPoint.zip

[ericoporto]

Mouse bounds

First the mouse.x and mouse.y boundary issue, it only happens if I move the mouse close to the game border, then click and hold the left click with it inside the game window, then I can move it outside the window boundaries. The fix mentioned above does prevent the script values to read outside values - the cursor can still go outside though, not sure if this matters or is expected behavior, but script wise, all things are ok.

Running with the actual 3.5.0.20 engine in this Ubuntu system, the in-game mouse cursor is never outside the window bounds, and consequent, neither the mouse script values. I tried the current master on Windows, and clicking and holding the mouse can't move the in-game cursor outside the window bounds there either. The OS cursor itself can move to outside in both cases, while the in-game cursor stays in place.

---

Screen.ScreenToRoomPoint

Now for the Screen.ScreenToRoomPoint issue, about the line in Engine/ac/screen.cpp. In my new test game, it doesn't ever return null.

But, when I try the old game, which is made with 3.5.0.20, I can still cause the issue. So I tried to debug again and I noticed in the old game, restrict is true.

But because the Script API reads like this

ags/Editor/AGS.Editor/Resources/agsdefns.sh

Lines 3069 to 3073 in 44e304f

	import static Point *ScreenToRoomPoint(int sx, int sy
	#ifdef SCRIPT_API_v36026
	, bool restrictToViewport = false
	#endif
	);

I thought the default value in old games was also false, but from above, it's actually true. So I think I just wasn't interpreting correctly what was the default value in previous versions. But maybe not?

ags/Engine/game/viewport.h

Line 154 in 14000e6

VpPoint ScreenToRoom(int scrx, int scry, bool clip = false, bool convert_cam_to_data = false) const;

In the commit (5014c50) that introduced the clip parameter, it's introduced with clipping defaulting to true in the 2 argument call. If it should be false, then it's just changing the last parameter here:

ags/Engine/ac/screen.cpp

Line 210 in 44e304f

ScriptUserObject* obj = Screen_ScreenToRoomPoint(params[0].IValue, params[1].IValue, true);

The change was introduced in v.3.6.0.34 and kept in v.3.6.0.35, so there aren't many releases put out with this behavior I think.

---

If necessary I can split the issue ticket.

[ivan-mogilko]

For Screen.ScreenToRoomPoint expected behavior is:

• Script API v36026 and above: restrictToViewport parameter is available and default value is false;
• Script API before v36026: restrictToViewport parameter is not available and internally this value is always true (always restrict), because that was the original behavior;

[ericoporto]

I am thinking that there's a chance the mouse working as is, is a behavior change in SDL 2.0.22, from it's wiki, issue libsdl-org/SDL#5050

While captured, mouse events still report coordinates relative to the current (foreground) window, but those coordinates may be outside the bounds of the window (including negative values).

And then later it mentions

Please note that as of SDL 2.0.22, SDL will attempt to "auto capture" the mouse while the user is pressing a button; this is to try and make mouse behavior more consistent between platforms, and deal with the common case of a user dragging the mouse outside of the window.

Noticeable, despite the notice, the behavior isn't consistent at all, since it's not what I am seeing on Windows. I need more experiments to check if this is an upstream regression.

It looks like this new behavior could be prevented using SDL_HINT_MOUSE_AUTO_CAPTURE set to 0.

---

About mouse capture, there's the following behavior, if the mouse moves outside, should the in-game cursor move be restricted to the border but follow along (if out of Y axis on top, follow on X axis), be frozen at last position and back moving when mouse is back on top of the game area (this is what happens without auto capture), or be allowed to be outside game borders. Note game border may not match window if there's an aspect ratio or something else that causes black borders to appear.

[ivan-mogilko]

The original ags behavior was to disable cursor movement completely (not react to mouse updates) when it's away from the window. But if the mouse is considered to be "in" the window, just in outside coordinates, then I guess it's okay to do what you describe as "restricted to the border but follow along".

If necessary, then clamping engine's mouse coordinates may be done here:

ags/Engine/device/mousew32.cpp

Lines 93 to 106 in 084f3f2

	if (!switched_away && Mouse::ControlEnabled)
	{
	// Use relative mouse movement; speed factor should already be applied by SDL in this mode
	int rel_x, rel_y;
	ags_mouse_get_relxy(rel_x, rel_y);
	real_mouse_x = Math::Clamp(real_mouse_x + rel_x, Mouse::ControlRect.Left, Mouse::ControlRect.Right);
	real_mouse_y = Math::Clamp(real_mouse_y + rel_y, Mouse::ControlRect.Top, Mouse::ControlRect.Bottom);
	}
	else
	{
	// Save real cursor coordinates provided by system
	real_mouse_x = sys_mouse_x;
	real_mouse_y = sys_mouse_y;
	}

It's interesting that clamping is already done for relative movement, but I don't remember why it was not done for an absolute one as well.

[ericoporto]

I verified the following does work! It gives the following behavior, the mouse cursor can leave the window, but the in-game cursor remains inside, and follow along, if you have the mouse button down, but since it's grabbed by the window, the OS cursor doesn't show up, and the perception is the mouse is locked down in-game, as you release the left click, the OS cursor reveals itself. You don't have the mouse button down, then the follow along behavior is obvious as the os cursor is visible. I only checked in Ubuntu 16.04, not sure how is the behavior in Windows or elsewhere.

I think this is a safe default behavior that won't feel weird in a future where #1686 lands.

Had to cast as int because the volatile qualifier caused a compilation error.

diff --git a/Engine/device/mousew32.cpp b/Engine/device/mousew32.cpp
index 51047c9..aeb8176 100644
--- a/Engine/device/mousew32.cpp
+++ b/Engine/device/mousew32.cpp
@@ -101,8 +101,8 @@ void mgetgraphpos()
     else
     {
         // Save real cursor coordinates provided by system
-        real_mouse_x = sys_mouse_x;
-        real_mouse_y = sys_mouse_y;
+        real_mouse_x = Math::Clamp((int) sys_mouse_x, Mouse::ControlRect.Left, Mouse::ControlRect.Right);
+        real_mouse_y = Math::Clamp((int) sys_mouse_y, Mouse::ControlRect.Top, Mouse::ControlRect.Bottom);
     }
 
     // Set new in-game cursor position

I am not sure how to deal with the code that follows this, that does some boundaries checks... If my understanding is correct, it's not needed.

[ericoporto]

I can reproduce this in MacOS too. :/