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

POC fix : Return previous callbacks (set by C) #373

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

folays
Copy link

@folays folays commented Mar 13, 2023

Hello,

Behaviour of go-gl/glfw hurts composability when registering input callbacks after 3rdparty C code would have already registered some callbacks.

My use case is :

ImGUI sets some callbacks here : https://github.com/ocornut/imgui/blob/v1.89.3/backends/imgui_impl_glfw.cpp#L451-L458

bd->PrevUserCallbackCursorPos = glfwSetCursorPosCallback(window, ImGui_ImplGlfw_CursorPosCallback);

ImGui plays nice and set some flags : https://github.com/ocornut/imgui/blob/v1.89.3/imgui.h#L1995-L1996

bool WantCaptureMouse;    // Set when Dear ImGui will use mouse inputs, in this case do not dispatch them to your main game/application (either way, always pass on mouse inputs to imgui). (e.g. unclicked mouse is hovering over an imgui window, widget is active, mouse was clicked over an imgui window, etc.).
bool WantCaptureKeyboard; // Set when Dear ImGui will use keyboard inputs, in this case do not dispatch them to your main game/application (either way, always pass keyboard inputs to imgui). (e.g. InputText active, or an imgui window is focused and navigation is enabled, etc.).

If you Go app comes "after" ImGui (registers some Go GLFW's callbacks AFTER ImGui) , your app is supposed to :

  • call the "previous" callbacks (belonging to ImGui)
  • then check those ImGui flags (WantCaptureMouse, WantCaptureKeyboard) which would let you known if ImGui had an usage of those inputs
    • (if you click on a ImGui window/widget, you aren't really clicking in your "below widgets" app, you wouldn't fire a raytrace to figure out where / on what object in the 3D world you clicked on...)
  • if those flags are sets, then do NOT dispatch the inputs to your app (return early)

HOWEVER, currently go-gl/glfw act as if there would never had been any C callbacks previously set :
https://github.com/go-gl/glfw/blob/master/v3.3/glfw/input.go#L685-L695

func (w *Window) SetCursorPosCallback(cbfun CursorPosCallback) (previous CursorPosCallback) {
	previous = w.fCursorPosHolder
	w.fCursorPosHolder = cbfun
	if cbfun == nil {
		C.glfwSetCursorPosCallback(w.data, nil)
	} else {
		C.glfwSetCursorPosCallbackCB(w.data)
	}
	panicError()
	return previous
}

As seen above, go-gl/glfw would only return a "previous" Go function from what it "remembered" to have "PERSONALLY ITSELF" set actually in w.fCursorPosHolder

Unless I'm missing something, it would be more sensical to let GLFW (C) directly inform us what was the "previous" callback, in case some were already set from C before.

I Proof-of-Concept'ed the proposed PR fix, as a short example only on SetCursorPosCallback ;

As you can see, the main changes for SetCursorPosCallback() are :

  • get rid of the C glfwSetCursorPosCallbackCB() (the *CB variant)
    • declare the Go's goCursorPosCB() (//exported)) in the Go's import "C" so the above wrapper is no longer needed
    • just call C.glfwSetCursorPosCallback() with the newly-declared goCursorPosCB() (instead of going trough the now-useless glfwSetCursorPosCallbackCB() wrapper)
  • introduce C goCursorPosCB_call() to be able to "call a C function pointer from Go"
  • return the "previous" callback as coming directly from the "previous" C function returned by GLFW's...
    • ...just wrap it as a Go function going through the above goCursorPosCB_call() wrapper since Go cannot directly call C function pointer (the Go function records the C function pointer as a closure)

Result : it works on my setup (macOS, Intel, Go 1.20) ; ImGui happily gets the events since from Go I can now just :

// preparation 1 : (3rd party : ImGui)
... let ImGui GLFW'ing createWindow() and set its own callbacks
// preparation 2 : (main app)
previousCallbacks.cursorPos = window.SetCursorPosCallback(_inputMousePos)

// actual inputs occurring ;
if previousCallbacks.cursorPos != nil {
	previousCallbacks.cursorPos(w, xpos, ypos)
}
if (ImGui's IO's WantCaptureMouse flag == true) {
	return // ImGui handled this input, stop further processing
}

If you agree that the fix is correct, I would update the PR for the others callbacks.

King Regards,

@folays
Copy link
Author

folays commented Mar 13, 2023

As a side note, some stuff have already been discussed here :

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

Successfully merging this pull request may close these issues.

1 participant