Game controller support #717

catmeow72 · 2024-09-12T18:40:35Z

This pull request adds controller support to the game.
TODO:

Add controller input
Read controller mappings
- Controller mappings from file in binary directory
- Controller mappings from environment variable
- Controller mappings from config
Download controller mappings
Add cursor sprite for controller cursor

… the window when clicking the buttons

…ing for button press detection on windows

…rity

… check in the process

Allow saving and configuring gamepad mappings Add gamepad sensitivity, for camera only Adjust multipliers in usage of gamepad axes Prompt for downloading controller mappings, and allow configuration

… duplicate code, or have to detect if startup has finished

IntegratedQuantum

I haven't looked through the entire code yet, but about half-way through I discovered that I keep repeating myself, so please apply my comments to the remaining code.

I mainly highlighted some syntax changes to simplify the code or make it less error prone.
Most notably, please use defer.

Also I would suggest to use the stackAllocator for all local allocations. It's much faster than using the global allocator.

And finally: Only OutOfMemory errors are unreachable. Other errors, like for file and network code can be reached by e.g. missing network connection or incorrect file permission. These errors must be handled and printed, otherwise we end up in hard to debug situations where the game just crashes and we have no idea why.

src/graphics/Window.zig

IntegratedQuantum

Sorry for taking so long to review.
I still think you could do some improvements here.
Could also explain the intentions behind this new cursor you added?

IntegratedQuantum · 2024-09-20T18:14:23Z

src/graphics/Window.zig

+const Gamepad = struct {
+	gamepadState: std.AutoHashMap(c_int, *c.GLFWgamepadstate) = undefined,
+	controllerMappingsDownloaded: bool = false,
+	controllerMappingsTask: ?ControllerMappingDownloadTask = null,


Since this has only one instance, you might as well just make these public variables.

IntegratedQuantum · 2024-09-20T18:30:16Z

src/graphics/Window.zig

+					self.gamepadState.put(jid, main.globalAllocator.create(c.GLFWgamepadstate)) catch unreachable;
+				}
+				_ = c.glfwGetGamepadState(jid, self.gamepadState.get(jid).?);
+				const oldState: c.GLFWgamepadstate = oldGamepadState orelse std.mem.zeroes(c.GLFWgamepadstate);


Why do you even have these twice, like you can just store the std.mem.zeroes(c.GLFWgamepadstate) in oldGamepadState and not make it optional.

IntegratedQuantum · 2024-09-20T18:45:48Z

src/graphics/Window.zig

+								key.value = newAxis;
+							}
+						}
+					}


This looks like it's almost the exact same code as 65-121
Is it possible to combine these into one?
It seems to be more error prone the way it is right now. For example you forgot to apply the deadzone in this version.

IntegratedQuantum · 2024-09-20T18:48:54Z

src/graphics/Window.zig

+				}
+				if(GLFWCallbacks.currentPos[1] >= winSize[1]) {
+					GLFWCallbacks.currentPos[1] = winSize[1] - 1;
+				}


There is no need to write it this verbose. What you are doing here is just a clamp on a vector. You can use std.math.clamp for that.

IntegratedQuantum · 2024-09-20T18:56:24Z

src/graphics/Window.zig

+	}
+	const ControllerMappingDownloadTask = struct { // MARK: ControllerMappingDownloadTask
+		curTimestamp: i128,
+		controllerMappingsDownloaded: *bool,


This construct seems a bit weird.
Instead of scheduling multiple tasks and removing the duplicate ones after the first one finished, you should just not schedule multiple tasks in the first place. This should be simpler to implement too.

(also for atomics it would be better ot use std.atomic.Value, to make this more type safe)

I already had it so that it doesn't schedule multiple tasks, but in a different function. I'll add this to the schedule function as well.

IntegratedQuantum · 2024-09-20T19:05:59Z

src/graphics/Window.zig

+			if (files.openDir(".") catch null) |dir| {
+				var _dir: files.Dir =dir;
+				if (_dir.hasFile("gamecontrollerdb.stamp")) {
+					const stamp = _dir.read(main.globalAllocator, "gamecontrollerdb.stamp") catch unreachable;


This error is not unreachable! The file might for example have incorrect permissions.
also: stackAllocator

IntegratedQuantum · 2024-09-20T19:08:26Z

src/graphics/Window.zig

+						timestamp = newTimestamp;
+					}
+				}
+			}


Here a block would be more readable in my opinion:

const timestamp: i128 = blk: { var dir = files.openDir(".") catch break :blk 0; if(!dir.hasFile(...)) break :blk 0; const stamp = _dir.read(main.stackAllocator, "gamecontrollerdb.stamp") catch break :blk 0; defer main.stackAllocator.free(stamp); break :blk std.fmt.parseInt(i128, stamp, 16) catch 0; };

IntegratedQuantum · 2024-09-20T19:10:54Z

src/graphics/Window.zig

+	pub fn init() *Gamepad {
+		const self: *Gamepad = main.globalAllocator.create(Gamepad);
+		if (!settings.askToDownloadControllerMappings) {
+			self.*.downloadControllerMappings();


You don't need to do .* when accessing members of a pointer.
This is Zig, not C/C++.

IntegratedQuantum · 2024-09-20T19:11:56Z

src/graphics/Window.zig

+			self.*.downloadControllerMappings();
+		}
+		self.*.updateControllerMappings();
+		self.*.gamepadState = std.AutoHashMap(c_int, *c.GLFWgamepadstate).init(main.globalAllocator.allocator);


As of the new zig version that I updated to recently, you can just use

self.*.gamepadState = .init(main.globalAllocator.allocator);

catmeow72 added 23 commits September 7, 2024 11:34

Refactor getting X positions of buttons in preparation for not moving…

4b7fe58

… the window when clicking the buttons

Prevent moving the window by the buttons

fa0cfaf

Use tabs instead of spaces for indentation, done using sed.

4f7ac23

Fix formatting actually now with sed in while loop

38a127b

Attempt to fix indentation

5826d95

Add minimum width and change return type of getButtonPositions

fc4fdc6

Merge branch 'PixelGuys:master' into no-drag-on-window-btn

ad780c4

Include window ID in size error

1349e7c

Fix orientation lines showing when clicking button and incorrect scal…

34166a7

…ing for button press detection on windows

Don't include windows without titlebar in window size requirement

9cd94e1

Merge branch 'PixelGuys:master' into no-drag-on-window-btn

020d86c

Take into account closability of windows when calculating min width

81bb5c4

Merge branch 'PixelGuys:master' into no-drag-on-window-btn

bb7d894

Add single quotes around window ID in the width error message for cla…

066ae02

…rity

Fix chat window automatic sizing, refactoring the minimum window size…

5334c13

… check in the process

Initial support for controllers

f28f15b

Basic controller support

21f855a

Merge branch 'PixelGuys:master' into controller

6f38fac

Allow runtime download of gamecontrollerdb.txt

44fc09c

Add new gamepad cursor asset, and use it.

cff55d4

Various improvements

b9e7fbb

Allow saving and configuring gamepad mappings Add gamepad sensitivity, for camera only Adjust multipliers in usage of gamepad axes Prompt for downloading controller mappings, and allow configuration

Refactor startup window handling so that controller settings does not…

c88f819

… duplicate code, or have to detect if startup has finished

Clamp cursor position to window size when changing via gamepad

ec39ef4

catmeow72 marked this pull request as ready for review September 13, 2024 16:52

catmeow72 added 2 commits September 16, 2024 08:55

Merge branch 'PixelGuys:master' into controller

02dd0e5

Add deadzone

7b97405

IntegratedQuantum requested changes Sep 16, 2024

View reviewed changes

catmeow72 added 3 commits September 16, 2024 12:17

Implement changes requested

9f74435

Follow more suggestions

e22274d

Use atomics

0acdb8e

Use orelse for oldState in Gamepads.update

806557d

IntegratedQuantum requested changes Sep 20, 2024

View reviewed changes

catmeow72 added 2 commits September 20, 2024 13:30

Implement requested changes

c6ba26c

Merge branch 'PixelGuys:master' into controller

7243f2a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Game controller support #717

Game controller support #717

catmeow72 commented Sep 12, 2024 •

edited

Loading

IntegratedQuantum left a comment

IntegratedQuantum left a comment

IntegratedQuantum Sep 20, 2024

IntegratedQuantum Sep 20, 2024

IntegratedQuantum Sep 20, 2024

IntegratedQuantum Sep 20, 2024

IntegratedQuantum Sep 20, 2024

catmeow72 Sep 20, 2024

IntegratedQuantum Sep 20, 2024

IntegratedQuantum Sep 20, 2024

IntegratedQuantum Sep 20, 2024

IntegratedQuantum Sep 20, 2024

Game controller support #717

Are you sure you want to change the base?

Game controller support #717

Conversation

catmeow72 commented Sep 12, 2024 • edited Loading

IntegratedQuantum left a comment

Choose a reason for hiding this comment

IntegratedQuantum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

catmeow72 commented Sep 12, 2024 •

edited

Loading