imgui icon indicating copy to clipboard operation
imgui copied to clipboard

[emscripten + glfw]: Added support for GLFW3 contrib port

Open ypujante opened this issue 1 year ago • 10 comments

This is the PR implementing the changes as discussed in PR #7520. Due to the changes + merge conflicts, I decided to start clean and do a brand new PR.

As discussed, this PR adds support for the GLFW3 contrib port, but does NOT force the user to switch to it.

  • I have not changed Makefile.emscripten in example_glfw_wgpu and example_glfw_opengl3 and as a result, building using these Makefiles, will build the same code as before
  • I have changed CMakeLists.txt for example_glfw_wgpu to automatically detect the version of emscripten used and if it can use the new port, it uses it: this gives an example on how to build with the new port
  • As discussed I also took the opportunity to fix #7600 (using the canvas selector as opposed to the document addresses the issue)

The most notable changes visible when using the GLFW3 contrib port are (which can be tested with the live demos)

  • the gamepad is now working properly
  • copy to clipboard works (Menu: Tools / About Dear ImGui / Config/Build Configuration / Copy to clipboard)
  • Hi DPI by default

Live demos:

  • Build using -sUSE_GLFW=3: demo
  • Build using --use-port=contrib.glfw3: demo

ypujante avatar Jun 02 '24 14:06 ypujante

Thank you for updating this.

EMSCRIPTEN_USE_GLFW3 seems a little confusingly named. Maybe I would use a more explicit name: EMSCRIPTEN_USE_PORT_CONTRIB_GLFW3 vs EMSCRIPTEN_USE_EMBEDDED_GLFW3 ?

I'll read the rest when I have some time.

ocornut avatar Jun 03 '24 16:06 ocornut

I agree it is a bit confusing. I renamed it per your suggestion.

ypujante avatar Jun 03 '24 16:06 ypujante

  • Why is emscripten_set_wheel_callback called in the ImGui_ImplGlfw_InstallEmscriptenCanvasResizeCallback() function?
  • I think the printf("") may be replaced by an assert() with comments.

ocornut avatar Jun 10 '24 12:06 ocornut

  • Why is emscripten_set_wheel_callback called in the ImGui_ImplGlfw_InstallEmscriptenCanvasResizeCallback() function?

You asked me to fix the #7600 issue for the embedded path and the fix is to call emscripten_set_wheel_callback with the selector. The prior code was calling emscripten_set_wheel_callback with a static value (for the document) and was being called in ImGui_ImplGlfw_Init. There is no access to the selector (in the embedded path) until you call ImGui_ImplGlfw_InstallEmscriptenCanvasResizeCallback() while providing it as a parameter. So that is the reason why I had to move it here.

* I think the `printf("")` may be replaced by an assert() with comments.

Ok I will change it.

ypujante avatar Jun 10 '24 12:06 ypujante

Thanks! I didn't realize this was the fix for #7600. I'll try to test this now.

ocornut avatar Jun 10 '24 12:06 ocornut

I wonder if we should:

  • rename ImGui_ImplGlfw_InstallEmscriptenCanvasResizeCallback() to ImGui_ImplGlfw_InstallEmscriptenCallbacks()
  • we can easily add an inline redirect under #ifndef IMGUI_DISABLE_OBSOLETE_FUNCTIONS block.
  • KEEP calling ImGui_ImplGlfw_InstallEmscriptenCallbacks() even for your port. We disable the code inside imgui_impl_glfw.cpp but it seems sane/safe to request users to call this so we have a backup path if ever some function in your port requires extra code?

What is the "window" string passed to emscripten_glfw_make_canvas_resizable() ? is that part of the template?

Would it make some sense to ask both window and canvas names in ImGui_ImplGlfw_InstallEmscriptenCallbacks() and do that automatically?

ocornut avatar Jun 10 '24 12:06 ocornut

I made the change you requested printf -> assert.

I made it as a static_assert because the runtime assert was pretty horrible from a user perspective

Screenshot 2024-06-10 at 05 59 18

At least with the static_assert the error message is at compilation time and add some detail:

../../backends/imgui_impl_glfw.cpp:867:19: error: static assertion failed: [ImGui_ImplGlfw_InstallEmscriptenCanvasResizeCallback] When using --use-port=contrib.glfw3, you should include <GLFW/emscripten_glfw3.h> and call emscripten_glfw_make_canvas_resizable instead
  867 |     static_assert(false, "[ImGui_ImplGlfw_InstallEmscriptenCanvasResizeCallback] When using --use-port=contrib.glfw3, you should include <GLFW/emscripten_glfw3.h> and call emscripten_glfw_make_canvas_resizable instead");
      |                   ^~~~~

In regards to your last questions:

  • rename ImGui_ImplGlfw_InstallEmscriptenCanvasResizeCallback() to ImGui_ImplGlfw_InstallEmscriptenCallbacks()
  • we can easily add an inline redirect under #ifndef IMGUI_DISABLE_OBSOLETE_FUNCTIONS block.
  • KEEP calling ImGui_ImplGlfw_InstallEmscriptenCallbacks() even for your port. We disable the code inside imgui_impl_glfw.cpp but it seems sane/safe to request users to call this so we have a backup path if ever some function in your port requires extra code?

I don't think I have an opinion one way or another. I don't envision the port to ever require something that would need to be added in ImGui_ImplGlfw_InstallEmscriptenCallbacks because it would mean that every other (non ImGui) projects my port is used would require this callbacks to be installed as well and I don't think that would be good for the end user.

But it doesn't cost much to do it the way you suggest and I am fine with it if that is the direction you want to go with.

What is the "window" string passed to emscripten_glfw_make_canvas_resizable() ? is that part of the template?

The documentation explains in detail the parameters provided to this function which can handle 3 different use cases. The "normal" use case for ImGui is to use the full window, and this corresponds to the css selector "window". As you can see in my WIP videos on a new project I am working on, (WIP1 and WIP2), in this instance I DON'T use "window" but the canvas selector itself, thus having ImGui run in a resizable frame in the browser.

Would it make some sense to ask both window and canvas names in ImGui_ImplGlfw_InstallEmscriptenCallbacks() and do that automatically?

I think the issue is that, as I mentioned, with the new API you don't necessarily use the full window, so I am not sure how the user could do that. And how would you pass the 3rd parameter (which is the handle to resize the canvas when you want one)?

ypujante avatar Jun 10 '24 13:06 ypujante

After spending more time thinking about your proposed changes (add ImGui_ImplGlfw_InstallEmscriptenCallbacks and deprecate ImGui_ImplGlfw_InstallEmscriptenCanvasResizeCallback) I tried to implement it and I think I really like the suggestion.

It makes it easier for regular ImGui user. Although I was worried about the fact that the emscripten_glfw_make_canvas_resizable allow for 3 different use cases (that are not covered with this solution), the reality is probably 99.9% of ImGui users will not care about not being full window. For the remaining users who want a more advanced control, this solution will still work as a user can write something like this:

// main.cpp
#ifdef __EMSCRIPTEN__
#include <GLFW/emscripten_glfw3.h>
#endif
// ....
#ifdef __EMSCRIPTEN__
    ImGui_ImplGlfw_InstallEmscriptenCallbacks(window, "#canvas");  // this will make it full window
    emscripten_glfw_make_canvas_resizable(window, "#xxx", "#yyy"); // this will make it resizable via #xxx and handle #yyy
#endif

In other words, my code supports calling emscripten_glfw_make_canvas_resizable multiple times and does the right thing...

In conclusion, I would go for this solution which covers 99.9% of the use cases while still allowing to use the more advanced usage. It is less discoverable, but I am planning to write examples demonstrating it (including open sourcing the project I linked to in my previous message).

I marked ImGui_ImplGlfw_InstallEmscriptenCanvasResizeCallback deprecated which then raises a nice warning during compilation:

main.cpp:108:5: warning: 'ImGui_ImplGlfw_InstallEmscriptenCanvasResizeCallback' is deprecated: Use ImGui_ImplGlfw_InstallEmscriptenCallbacks instead [-Wdeprecated-declarations]
  108 |     ImGui_ImplGlfw_InstallEmscriptenCanvasResizeCallback("#canvas");
      |     ^
../../backends/imgui_impl_glfw.h:35:3: note: 'ImGui_ImplGlfw_InstallEmscriptenCanvasResizeCallback' has been explicitly marked deprecated here
   35 | [[deprecated("Use ImGui_ImplGlfw_InstallEmscriptenCallbacks instead")]]
      |   ^
1 warning generated.

ypujante avatar Jun 10 '24 17:06 ypujante

Hi @ocornut, I just found out an issue which is reproducible in BOTH the old/embedded path as well as the new one on macOS

  • go to Demo/Inputs & Focus
  • Hit Meta (Super)
  • While holding Meta, Press V
  • Release V
  • Release Meta

"V" is still pressed

Meta_V

I am going to investigate of why this is happening and I will let you know what I find.

ypujante avatar Jun 23 '24 18:06 ypujante

As I pointed out in my comment:

Since this problem existed before I don't see a reason why this should prevent this PR to be merged (unless there are other reasons to prevent the merge of course).

ypujante avatar Jun 25 '24 17:06 ypujante

I merged all the changes (I saw there was a conflict which I fixed). I also rebuilt/republished the demo built with the latest emscripten (1.3.62) which contains the latest version of emscripten-glfw.

In particular this update contains the following changes:

  • addresses the Meta key issue on macOS as mentioned above
  • allow for copy to clipboard (so you can now copy the configuration to the clipboard)
Dear ImGui 1.91.0 WIP (19093)
--------------------------------
sizeof(size_t): 4, sizeof(ImDrawIdx): 2, sizeof(ImDrawVert): 20
define: __cplusplus=201703
define: IMGUI_DISABLE_FILE_FUNCTIONS
define: __GNUC__=4
define: __clang_version__=19.0.0git (https:/github.com/llvm/llvm-project c00ada070207979f092be9046a02fcfff8b9f9ce)
define: __EMSCRIPTEN__
--------------------------------
io.BackendPlatformName: imgui_impl_glfw
io.BackendRendererName: imgui_impl_webgpu
io.ConfigFlags: 0x00000003
 NavEnableKeyboard
 NavEnableGamepad
io.ConfigInputTextCursorBlink
io.ConfigWindowsResizeFromEdges
io.ConfigMemoryCompactTimer = 60.0
io.BackendFlags: 0x0000000E
 HasMouseCursors
 HasSetMousePos
 RendererHasVtxOffset
--------------------------------
io.Fonts: 1 fonts, Flags: 0x00000000, TexSize: 512,64
io.DisplaySize: 1232.00,688.00
io.DisplayFramebufferScale: 2.00,2.00
--------------------------------
style.WindowPadding: 8.00,8.00
style.WindowBorderSize: 1.00
style.FramePadding: 4.00,3.00
style.FrameRounding: 0.00
style.FrameBorderSize: 0.00
style.ItemSpacing: 8.00,4.00
style.ItemInnerSpacing: 4.00,4.00

ypujante avatar Jul 06 '24 17:07 ypujante

Is this mergeable? Topics/PR going over multiple things tends to be overwhelming/hard to process for me. I'll just need to fix the coding style but I can do that myself. I will also likely nuke the obsolete stuff and turn them into comments, there's too much too maintain already and Emscripten is fast moving.

ocornut avatar Jul 08 '24 13:07 ocornut

As far as I know yes, this is mergeable. Due to the fact that you have to opt-in to use the new implementation, and the old implementation is still the default, it should be fairly low risk in the gran scheme of things.

ypujante avatar Jul 08 '24 13:07 ypujante

Topics/PR going over multiple things tends to be overwhelming/hard to process for me.

Let me try to recap it for you and summarize it. This PR includes:

  1. Introduced ability to swap the emscripten GLFW implementation from the embedded one (javascript) to the more modern/fully featured external one (cpp):
    • multi-window (glfw/canvas) support
    • 3 different ways of resizing supported
    • gamepad actually working (broken in embedded)
    • all glfw cursors
    • copy to clipboard
    • workaround for Super/Meta key (broken in embedded) (#7732)
    • much more...
  2. Embedded implementation is still the default and can always be used. The CMakeLists.txt for examples_glfw_wgpu was changed to detect if the new implementation can be used and if so uses it (can be overridden to revert the behavior, documented in the file) .
  3. Fixed the warning from #7600 in the embedded path

ypujante avatar Jul 08 '24 14:07 ypujante

It also breaks GLFW compilation without Emscripten. I'll finish this and split into two commits.

ocornut avatar Jul 08 '24 19:07 ocornut

I sincerely apologize for that oversight. If you want me to address the problem I'm more than happy to look into it

ypujante avatar Jul 08 '24 19:07 ypujante

No worries I'll finish it, but it's always easier when PR are separated in commits. Here I'll want separate the contrib port support from the ImGui_ImplGlfw_InstallEmscriptenCallbacks() rename.

ocornut avatar Jul 08 '24 19:07 ocornut

Merged as 6816789 and 2937339 (with various small changes) I got rid of ImGui_ImplGlfw_InstallEmscriptenCanvasResizeCallback() redirect because we have too much cruft, Emscripten is sort of a moving target, and the fix on user's end is trivial. It is kept as a comment! Thanks!

ocornut avatar Jul 08 '24 20:07 ocornut

@ocornut Thank you for merging my changes and again, I apologize for the issues. I am not sure if I could have triggered a CI run myself but it would have helped in knowing I had broken something. I will do better next time.

I also wanted to reiterate that you should not hesitate to contact me if there is any issue with the code changes that I did and I will do my best to respond as quickly as possible. I can also help you with questions about emscripten in general (like you involved me for the "open a window" feature) and will be more than happy to help if I can.

ypujante avatar Jul 08 '24 20:07 ypujante

GitHub doesn’t seem to run CI without approval unless a commit(?) or pr(?) has previously been merged from the same author. But i am not sure which rule uses precisely: as I tend to amend commits sometimes github doesn’t consider a PR as merged even if it is. I could have validated the PR earlier for CI but I also tend to forget to do this before I get around to review something.

I also forgot to add the glfw contrib mention to the main changelog.txt i will do this on a future commit (i am afk now).

Thanks for your offer to help, much appreciated.

ocornut avatar Jul 08 '24 20:07 ocornut

Pushed additional fix a8e96ae (caused by my own leftover during a test)

ocornut avatar Jul 08 '24 20:07 ocornut

@ocornut I just wanted to let you know that I did a sanity check after you made my changes to master (a8e96ae21a4ec10e5f02b19dd865dfffe8a98e67) and everything looks fine with both embedded/port backends (I tested Makefile.emscripten and CMakeLists.txt)

In case you are interested, I think there is a piece of information that could be added to the Config/Build Information of the About window: the version of emscripten used to compile (since issues could be tied to which version of emscripten is used to compile...). It is very easy to get this data:

#include <emscripten/version.h>
// ...
ImGui::Text("emscripten: %d.%d.%d\n", __EMSCRIPTEN_major__, __EMSCRIPTEN_minor__, __EMSCRIPTEN_tiny__);

This defines are described in the documentation

ypujante avatar Jul 10 '24 13:07 ypujante

Hi, Nice work !

But " Build using --use-port=contrib.glfw3: demo" with my system: Linux / Chrome (with enabled WebGPU) I noticed problems:

  1. Do not resize to full-screen on "F11" press, it could be resized from Chrome menu -> full screen. resize back work on "F11" press. (old version "-sUSE_GLFW=3" work in both cases)

  2. Copy/Paste do not work in direction OtherApp (TextEditor/etc) -> Chrome work only in direction Chrome -> OtherApp (old version "-sUSE_GLFW=3" do not work copy/paste at all, but could work both directions with implemented extra functions I took here around)

Maksons avatar Aug 02 '24 11:08 Maksons

Hi, Nice work !

But " Build using --use-port=contrib.glfw3: demo" with my system: Linux / Chrome (with enabled WebGPU) I noticed problems:

1. Do not resize to full-screen on "F11" press, it could be resized from Chrome menu -> full screen.
   resize back work on "F11" press.
   (old version "-sUSE_GLFW=3" work in both cases)

I never use F11 (I am on macOS) so I didn't know that was even a thing. I am not sure what triggers going fullscreen on F11 with -sUSE_GLFW=3. I would suggest having a menu and/or a button in your application to request full screen. If you look at my demo I have button(s) to go full screen. There is documentation about full screen support in the library.

I am curious what makes F11 work with the other implementation. The good news is that if this is an issue for you, you can still revert to using the old implementation as ImGui supports both (it is not a replacement).

2. Copy/Paste do not work in direction OtherApp (TextEditor/etc) -> Chrome
   work only in direction Chrome -> OtherApp
   (old version "-sUSE_GLFW=3" do not work copy/paste at all,
   but could work both directions with implemented extra functions I took here around)

Yes I am aware of the problem, I actually opened an issue for this purpose yesterday. Since the latest version of emscripten-glfw (which is not yet available in emscripten but can be still be used manually) I added an API to retrieve the "external" clipboard but I am not sure how to integrate it in ImGui, hence the ticket opened. You can actually see it in action in my live application: the code editor, which I control, uses this new API and as a result allow copy/paste from OtherApp -> Chrome...

But as you say, the old version does not support copy/paste at all, so it is already better :)

ypujante avatar Aug 02 '24 12:08 ypujante

@Maksons I believe I understand why my library is "swallowing" F11. I will implement a way to allow enabling/disabling this kind of behavior in an upcoming release of emscripten-glfw

ypujante avatar Aug 08 '24 16:08 ypujante

@Maksons I just wanted to let you know that I have released the new version of emscripten-glfw that addresses the 2 issues you mentioned a couple of weeks ago. I have submitted a PR in emscripten to merge the new version but unfortunately I do not control the timing of release.

If you want to use it before it gets released in emscripten, you can take a look at my other project, WebGPU Shader Toy, which has turned ImGui into an emscripten port and uses the latest version of emscripten-glfw, without depending on the emscripten release cadence...

Clipboard

If you are on PC or Linux, copy from Clipboard will just work as long as you use the keyboard shortcuts. On macOS you simply add this line (see Usage):

ImGui::GetIO().ConfigMacOSXBehaviors = emscripten::glfw3::IsRuntimePlatformApple();

Note that I am planning to open a PR on ImGui to do this automatically, but I need to wait for emscripten to a) merge my changes, b) release a version that contains the merged changes...

F11 key

See the Keyboard support section documentation for details, but in a nutshell, you simply add this to your code:

emscripten::glfw3::AddBrowserKeyCallback([](GLFWwindow* window, int key, int scancode, int action, int mods) {
  return mods == 0 && action == GLFW_PRESS && key == GLFW_KEY_F11;
});```

ypujante avatar Aug 19 '24 18:08 ypujante