Odin icon indicating copy to clipboard operation
Odin copied to clipboard

Proposal: Make `vendor:glfw` more idiomatic Odin

Open sven-strothoff opened this issue 4 months ago • 1 comments

For the most part the GLFW bindings are a direct mapping of the original source files. For instance lots of #defines are mapped to constants in Odin.

I think there is room for improvement here. Adding more explicit type definitions would make the code using GLFW more idiomatic Odin code and catch bugs during compilation. I am mainly talking about grouping the long list of constants into sensible enums and adjusting the procedure definitions accordingly.

I made some initial tests and would be willing to go over the complete GLFW bindings and convert everything to contribute to the Odin project. However, I wanted to see if there are reasons against making a change like this before putting in all the effort.

See below for some examples.

Please let me know if you would be open to the proposed changes or if there is a reason against it, that I am not aware of. Thanks! :)

Example 1: GetKey

The constants are already grouped by comments in the source code (both original GLFW and vendor:glfw). So for instance the key states https://github.com/odin-lang/Odin/blob/8135dda2fc42b3c474f0a77d7f03126b2eb8c034/vendor/glfw/constants.odin#L16-L19 could be grouped as

KeyState :: enum i32 {
	RELEASE = 0,
	PRESS   = 1,
	REPEAT  = 2,
}

Introducing the KeyState enum from above and a Keys enum for all the key constants (left out for brevity) the definition of GetKey could be changed from https://github.com/odin-lang/Odin/blob/d4a1670b93a174056b309db7e0e851e5e516bccb/vendor/glfw/bindings/bindings.odin#L116 to

GetKey :: proc(window: WindowHandle, key: Keys) -> KeyState ---

The usage code would change from

// before
if glfw.GetKey(window, glfw.KEY_SPACE) == glfw.PRESS {
	fmt.println("space is pressed")
}

to

// after
if glfw.GetKey(window, .KEY_SPACE) == .PRESS {
	fmt.println("space is pressed")
}

Example 2: SetInputMode

Turning https://github.com/odin-lang/Odin/blob/8135dda2fc42b3c474f0a77d7f03126b2eb8c034/vendor/glfw/constants.odin#L346-L356 into enums InputMode and CursorMode

/* Cursor draw state and whether keys are sticky */
InputMode :: enum i32 {
	CURSOR               = 0x00033001,
	STICKY_KEYS          = 0x00033002,
	STICKY_MOUSE_BUTTONS = 0x00033003,
	LOCK_KEY_MODS        = 0x00033004,
}

/* Cursor draw state */
CursorMode :: enum i32 {
	CURSOR_NORMAL   = 0x00034001,
	CURSOR_HIDDEN   = 0x00034002,
	CURSOR_DISABLED = 0x00034003,
	CURSOR_CAPTURED = 0x00034004,
}

would allow the definition of SetInputMode to be changed from https://github.com/odin-lang/Odin/blob/d4a1670b93a174056b309db7e0e851e5e516bccb/vendor/glfw/bindings/bindings.odin#L148 to the more expressive

SetInputMode :: proc (window: WindowHandle, mode: InputMode, value: union { CursorMode, b32 }) ---

The usage code would change from

// before
glfw.SetInputMode(window, glfw.CURSOR, glfw.CURSOR_NORMAL)
glfw.SetInputMode(window, glfw.STICKY_KEYS, 1) // See below

to

// after
glfw.SetInputMode(window, .CURSOR, .CURSOR_NORMAL)
glfw.SetInputMode(window, .STICKY_KEYS, true)

Bonus fix regarding the 1 above: Using glfw.TRUE (like in C) here does not work as glfw.TRUE is defined as true in Odin, but SetInputMode expects an i32 here. After my proposed change true (or glfw.TRUE if you like) can be used here.

sven-strothoff avatar Jun 02 '25 16:06 sven-strothoff