Try / Tempest

3d graphics engine
MIT License
83 stars 24 forks source link

Partial window focus implementation #42

Closed Reveares closed 1 year ago

Reveares commented 1 year ago

Provides implementation in SystemAPI to check window focus for Windows and X11. MacOS excluded, because I have no idea about it. See https://github.com/Try/OpenGothic/issues/374

Try commented 1 year ago

Hi, @Reveares thank for raising it up!

I'm sorry, but this kind of implementation cannot be merged as-is, because it basically bypasses UI sub-system entirely.

Before approaching there is few thing, that require behavior clarification:

  1. What return value of Widget::hasFocus() would be, if parent window lost focus ?
  2. Should we emit Widget::focusEvent(FocusEvent&) ?
  3. How to restore focus?
  4. What to do if Widget::setFocus gets called, when parent window is out of focus?
Reveares commented 1 year ago

You bring up good points, but I think these are two different problems we want to solve in the end. This PR is to provide an API for the main window when the user or OS changes the input focus away from the main window. In such a case it should not be possible for the engine to get the focus back directly, so Widget::setFocus should do nothing for the main window. Widget::hasFocus() would always return false, since the application is no longer in focus. Emitting an event would certainly make sense.

But what this PR doesn't solve are the points you raised about what should happen inside the engine in the widget system.

With the PR you could check in OpenGothic in https://github.com/Try/OpenGothic/blob/df2e659bfb484409a3fd8303f84ccf0c9a4ade62/game/mainwindow.cpp#L273 whether you have input focus from the OS.

Would you agree with that? Otherwise I am of course open for alternatives.

Try commented 1 year ago

This PR is to provide an API for the main window when

SystemApi::isFocused in PR is new api that contradicts existing one, isn't it? Basically have a assertion, like assert(SystemApi::isFocused(w.hwnd()) == w.hasFocus()) wont holdup. If your goal is short-term solution, why not it make more sense to call Windows-API directly from the game, isn't it?

From engine side it makes more sense to call for window->implSetFocus, from WM_SETFOCUS event. (except it wont work as-is for receiving focus back)

I've tested in small Qt app, to see how focus handled by them:

So far idea is: Implement WM_SETFOCUS based solution, translate it to EventDispatcher, similarly to any else event. Build some mechanism to restore focus. not sure how, maybe memorize last focused widget in EventDispatcher Ignore setFocus, when out-of-focus case.

Reveares commented 1 year ago

I understand that in the engine you prefer a correct implementation with the UI subsystem. Since I don't understand enough about the engine yet, I would implement the workaround in OpenGothic if that's ok with you.

Whilethis is not a problem for Windows, the implementation for linux does not work, because XGetInputFocus needs Display* as input, which is only available in https://github.com/Try/Tempest/blob/6c388c5b6e056235377ec796bbf9f5ef8a91ff8a/Engine/system/api/x11api.cpp#L47

What would be your suggestion to deal with this so that the workaround doesn't get too messy?

Edit: I overlooked X11Api::display()

Reveares commented 1 year ago

I think implementing the workaround only in OpenGothic is not possible. The following is the code I created

diff --git a/CMakeLists.txt b/CMakeLists.txt
index be207d6d..abccd94c 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -31,6 +31,8 @@ if(NOT MSVC)
   if(UNIX)
     # -rdynamic is needed to allow obtaining meaningful backtraces
     target_link_options(${PROJECT_NAME} PRIVATE -rdynamic)
+
+    target_link_libraries(${PROJECT_NAME} X11)
   endif()
 endif()

diff --git a/game/mainwindow.cpp b/game/mainwindow.cpp
index 1361a3cc..470f010d 100644
--- a/game/mainwindow.cpp
+++ b/game/mainwindow.cpp
@@ -26,6 +26,14 @@
 #include "commandline.h"
 #include "gothic.h"

+#ifdef __WINDOWS__
+#include <Windows.h>
+#elif __LINUX__
+#include <X11/Xlib.h>
+#undef CursorShape
+#undef Status
+#endif
+
 using namespace Tempest;

 MainWindow::MainWindow(Device& device)
@@ -270,7 +278,26 @@ void MainWindow::mouseDragEvent(MouseEvent &event) {
   }

 void MainWindow::mouseMoveEvent(MouseEvent &event) {
-  processMouse(event,SystemApi::isFullscreen(hwnd()));
+  // FIXME: Replace by implementing focus handling in the UI subsystem.
+  bool isWindowFocused = true;
+#ifdef __WINDOWS__
+  isWindowFocused = HWND(hwnd()) == GetActiveWindow();
+#elif __LINUX__
+  // Replicates the HWND class in x11api.cpp, which is not accessible
+  ::Window currentWindow;
+  std::memcpy(&currentWindow,hwnd(),sizeof(currentWindow));
+
+  // ERROR: Can't access X11API outside of Tempest!
+  auto* dpy = reinterpret_cast<Display*>(X11Api::display());
+
+  ::Window focusedWindow;
+  int revert;
+  if (XGetInputFocus(dpy, &focusedWindow, &revert)) {
+    isWindowFocused = currentWindow == focusedWindow;
+    }
+#endif
+  // TODO: Implement workaround for other platforms.
+  processMouse(event,SystemApi::isFullscreen(hwnd()) && isWindowFocused);
   }

 void MainWindow::processMouse(MouseEvent& event, bool enable) {

But Tempest does not export x11api.h which contains X11Api::display() but only <Tempest/SystemApi> that contains nothing platform specific.

Try commented 1 year ago

But Tempest does not export x11api.h which contains X11Api::display() but only <Tempest/SystemApi> that contains nothing platform specific.

Hm.. Right, current interface is not enough, to do much of platform-dependent work (at least on Linux). Would it be OK, with you, if I will implement proper focus handling into the engine for Windows, so you can use it as a reference for Linux solution?

Reveares commented 1 year ago

That would be fine with me.

Try commented 1 year ago

That would be fine with me.

Ready. Now on windows OpenGothic can be modified as:

void MainWindow::processMouse(MouseEvent& event, bool enable) {
  auto center = Point(w()/2,h()/2);
- if(enable && event.pos()!=center) {
+ if(enable && event.pos()!=center && hasFocus()) {
    dMouse += (event.pos()-center);
    setCursorPosition(center);
    }

On X11, FocusIn/FocusOut event should be implemented similar to WM_ACTIVATE