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

Emscripten: Uses macOS behavior on macOS (using glfw contrib port) #7915

Closed
wants to merge 4 commits into from

Conversation

ypujante
Copy link
Contributor

Hi @ocornut

This is the changes for the Issue #7851: "Emscripten: Handling "external" clipboard"

What this PR contains:

  1. changed CI test to also test the CMakeLists build for example_glfw_wgpu and emscripten: this tests the path when emscripten-glfw is used. Here is an example output (which shows the port being retrieved as well as the version):
Run pushd emsdk-master
~/work/imgui/imgui/emsdk-master ~/work/imgui/imgui
Setting up EMSDK environment (suppress these messages with EMSDK_QUIET=1)
Adding directories to PATH:
PATH += /home/runner/work/imgui/imgui/emsdk-master
PATH += /home/runner/work/imgui/imgui/emsdk-master/upstream/emscripten

Setting environment variables:
PATH = /home/runner/work/imgui/imgui/emsdk-master:/home/runner/work/imgui/imgui/emsdk-master/upstream/emscripten:/snap/bin:/home/runner/.local/bin:/opt/pipx_bin:/home/runner/.cargo/bin:/home/runner/.config/composer/vendor/bin:/usr/local/.ghcup/bin:/home/runner/.dotnet/tools:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/games:/usr/local/games
EMSDK = /home/runner/work/imgui/imgui/emsdk-master
EMSDK_NODE = /home/runner/work/imgui/imgui/emsdk-master/node/18.20.3_64bit/bin/node
~/work/imgui/imgui
emcc (Emscripten gcc/clang-like replacement + linker emulating GNU ld) 3.1.65 (7f8a05dd4e37cbd7ffde6d624f91fd545f7b52e3)
clang version 20.0.0git (https:/github.com/llvm/llvm-project 547917aebd1e79a8929b53f0ddf3b5185ee4df74)
Target: wasm32-unknown-emscripten
Thread model: posix
InstalledDir: /home/runner/work/imgui/imgui/emsdk-master/upstream/bin
configure: cmake -B build -DCMAKE_BUILD_TYPE=Release examples/example_glfw_wgpu -DCMAKE_TOOLCHAIN_FILE=/home/runner/work/imgui/imgui/emsdk-master/upstream/emscripten/cmake/Modules/Platform/Emscripten.cmake -DCMAKE_CROSSCOMPILING_EMULATOR=/home/runner/work/imgui/imgui/emsdk-master/node/18.20.3_64bit/bin/node
-- Using --use-port=contrib.glfw3 GLFW implementation
-- Configuring done (0.4s)
-- Generating done (0.0s)
-- Build files have been written to: /home/runner/work/imgui/imgui/build
[ 11%] Building CXX object CMakeFiles/example_glfw_wgpu.dir/main.cpp.o
ports:INFO: retrieving port: contrib.glfw3 from https://github.com/pongasoft/emscripten-glfw/releases/download/v3.4.0.20240817/emscripten-glfw3-3.4.0.20240817.zip
ports:INFO: unpacking port: contrib.glfw3
cache:INFO: generating port: sysroot/lib/wasm32-emscripten/lib_contrib.glfw3-O2.a... (this will be cached in "/home/runner/work/imgui/imgui/emsdk-master/upstream/emscripten/cache/sysroot/lib/wasm32-emscripten/lib_contrib.glfw3-O2.a" for subsequent builds)
system_libs:INFO: compiled 7 inputs in 5.25s
cache:INFO:  - ok
[ 22%] Building CXX object CMakeFiles/example_glfw_wgpu.dir/home/runner/work/imgui/imgui/backends/imgui_impl_glfw.cpp.o
[ 33%] Building CXX object CMakeFiles/example_glfw_wgpu.dir/home/runner/work/imgui/imgui/backends/imgui_impl_wgpu.cpp.o
[ 44%] Building CXX object CMakeFiles/example_glfw_wgpu.dir/home/runner/work/imgui/imgui/imgui.cpp.o
[ 55%] Building CXX object CMakeFiles/example_glfw_wgpu.dir/home/runner/work/imgui/imgui/imgui_draw.cpp.o
[ 66%] Building CXX object CMakeFiles/example_glfw_wgpu.dir/home/runner/work/imgui/imgui/imgui_demo.cpp.o
[ 77%] Building CXX object CMakeFiles/example_glfw_wgpu.dir/home/runner/work/imgui/imgui/imgui_tables.cpp.o
[ 88%] Building CXX object CMakeFiles/example_glfw_wgpu.dir/home/runner/work/imgui/imgui/imgui_widgets.cpp.o
[100%] Linking CXX executable index.js
cache:INFO: generating system asset: symbol_lists/0d471007c2d667e769d9b01db9c4a5c0306744ec.json... (this will be cached in "/home/runner/work/imgui/imgui/emsdk-master/upstream/emscripten/cache/symbol_lists/0d471007c2d667e769d9b01db9c4a5c0306744ec.json" for subsequent builds)
cache:INFO:  - ok
[100%] Built target example_glfw_wgpu
  1. Added the emscripten version to "About/Config" in the demo. I feel like it is a pretty important piece of information since there was somebody recently reporting an issue with emscripten and it was due to using an old version of emscripten.

  2. Use emscripten::glfw3::OpenURL if available. This changes fixes the "Popup blocked" message that you get with some browsers, in particular Safari when clicking on links in ImGui

  3. Automatically detect the runtime platform and if Apple, sets ImGui::GetIO().ConfigMacOSXBehaviors to true. Note that due to the issues with the Meta Key and the fact that ImGui is managing key repeat internally, it is a better user experience to simply "disable" repeat for Meta + C / X / V. I personally think that it is a fine tradeoff. This changes is required IF you would like to fix the issues with Clipboard copy/paste (Emscripten: Handling "external" clipboard #7851)

With the latest version of emscripten-glfw (3.4.0.20240817) available in emscripten 3.1.65 (released yesterday), you also get:

  • scrollbars now work outside the browser
  • copy/paste with natural shortcuts on Windows (and on macOS if you apply the changes recommended)

I pushed a live demo for this PR

Note that 1/2/3 and 4 are completely unrelated and I just bundled them into the same PR. If you would rather have separate PRs, I can do that.

Also if you feel like 4 is not the direction you want to go (automatically using macOS behavior), it is OK not to accept this PR at all. The changes are pretty minimal and obviously anybody can write these 2 lines after calling ImGui_ImplGlfw_InstallEmscriptenCallbacks

@ocornut
Copy link
Owner

ocornut commented Aug 23, 2024

Thanks!

Note that 1/2/3 and 4 are completely unrelated and I just bundled them into the same PR. If you would rather have separate PRs, I can do that.

I would prefer separate PR or at minimum a neat commit history with 1 feature per commit, and then I can cherry-pick them separately. Right now if I see 9 commits with description such as "trying again" it doesn't give me much confidence to be looking at them :)

@ypujante
Copy link
Contributor Author

I have updated this PR with 4 commits one for each item (I added the number in the commit). If you prefer 4 PRs, I can do that too.

@ocornut
Copy link
Owner

ocornut commented Aug 23, 2024

That’s good thanks!

ocornut pushed a commit that referenced this pull request Sep 3, 2024
ocornut pushed a commit that referenced this pull request Sep 3, 2024
…SCRIPTEN_USE_PORT_CONTRIB_GLFW3. Fixes popup blocked in some browsers. (#7915, #7660)
@ocornut
Copy link
Owner

ocornut commented Sep 3, 2024

(1) I thought the other test already tested for emscripten-glfw (not contrib)? So what's new about this? It is just for testing the cmakefile? I'm not even happy with that cmakefile it seems to contains the whole world :( (see #7435)

(2) Merged with minor changes as 07be017.

(3) Merged with minor changes 30dcdcb

(4) Should this be this done during Init ? At minimum since what this does is really unobvious it should include a comment detailing the whys, and perhaps issue # as well.
It also seems really important that we have a solution for Emscripten that doesn't rely on your contrib port?

Thanks again!

Some extra feedback:

  • I admit I am not super confident with the idea of pushing code specific to your GLFW3 Contrib Port if it doesn't eventually become a defacto standard. Do you see a viable path for this to completely replace Emscripten's own outdated version?
  • Please try to make an effort with coding style, casing, spacing if you can 🙇 It really helps me otherwise all my brain can see are those as small bits of noise and I'm struggling to move forward past them.

@ypujante
Copy link
Contributor Author

ypujante commented Sep 3, 2024

(1) I thought the other test already tested for emscripten-glfw (not contrib)? So what's new about this? It is just for testing the cmakefile? I'm not even happy with that cmakefile it seems to contains the whole world :( (see #7435)

No, the other tests do not test emscripten-glfw because Makefile.emscripten still compile with the built-in GLFW3 implementation (-sUSE_GLFW=3). Because there is some logic to detect if emscripten-glfw is available it was a bit complicated to implement in Makefile "language". Without this addition, the path "Emscripten + ImGui + emscripten-glfw" is not tested in CI.

For what it's worth, I do not think this CMakeFile contains the whole world... that is what CMakeFile look like...


(2) Merged with minor changes as 07be017.

Great. Thank you.


(3) Merged with minor changes 30dcdcb

Great. Thank you.


(4) Should this be this done during Init ? At minimum since what this does is really unobvious it should include a comment detailing the whys, and perhaps issue # as well. It also seems really important that we have a solution for Emscripten that doesn't rely on your contrib port?

As I mentioned for (4), you don't have to do it and you clearly seem to be very uncomfortable doing so (see your extra feedback) so I would suggest dropping it:

  • I thought you had mentioned that it was desirable for users on macOS to use the native keyboard shortcuts (like it is the case when building an ImGui desktop app for macOS).
  • I thought it would be desirable to have copy/paste working on macOS

The code added in (4) is trivial to add to any project, like I did to my own. I can certainly provide details in the bug I filed about copy/paste (#7851) so that people know what to do.


In regards to this comment "_It also seems really important that we have a solution for Emscripten that doesn't rely on your contrib port_", I am not sure what you are referring to exactly:
  • If you are referring to determining if the platform is Apple, then here is my code, feel free to implement it similarly to ImGui_ImplGlfw_EmscriptenOpenURL, there is no magic there...
  // JavaScript
  emscripten_glfw3_context_is_runtime_platform_apple: () => {
    return navigator.platform.indexOf("Mac") === 0 || navigator.platform === "iPhone";
  },

but be WARNED that the built-in implementation of GLFW3 does not handle the meta key properly (does not have the workaround) and as a result will break keyboard in ImGui, so it is really not recommended to switch to macos behavior with this implementation...

  • if you are referring to copy/paste, I have no desire to port my code to the built-in implementation of GLFW3

Thanks again!

Some extra feedback:

  • I admit I am not super confident with the idea of pushing code specific to your GLFW3 Contrib Port if it doesn't eventually become a defacto standard. Do you see a viable path for this to completely replace Emscripten's own outdated version?

I wished I had a crystal ball to tell you that. I have worked really hard to a) implement it, b) integrate it in emscripten so that it is readily available via a contrib port, c) integrate it in ImGui so that it can be used by others while leaving the ability to use the old implementation. But whether it becomes the de facto or not I don't know. I believe, by now, it is a far superior implementation, implementing as much of the spec and in as much compliance as possible (like you noticed when compiling with the contrib port you don't need most of the #ifdef statements required you had to use for the old implementation). Including pretty big features like joystick, clipboard, copy/paste, etc that are just not implemented/not working in the old one.

But believe me, I do understand your reticence and I am not offended by it. It is the nature of open source.

  • Please try to make an effort with coding style, casing, spacing if you can 🙇 It really helps me otherwise all my brain can see are those as small bits of noise and I'm struggling to move forward past them.

I did try really hard this time to comply with your coding style, which is not mine... and I guess I failed once more :( FWIW, the Emscripten project runs a CI phase to check for coding style and it fails the build if it doesn't match. You could also provide a clang format file that I could use in my IDE to help with this mundane yet so important details...

@ocornut
Copy link
Owner

ocornut commented Sep 3, 2024

For what it's worth, I do not think this CMakeFile contains the whole world... that is what CMakeFile look like...

I guess I was referring to need to build Dawn. It's not that part but I hope those tech will eventually settle.

I thought you had mentioned that it was desirable for users on macOS to use the native keyboard shortcuts
I thought it would be desirable to have copy/paste working on macOS

Absolutely!

I'm not uncomfortable with moving forward with (4) as it is useful, my comment requested a comment and wondering if it was done in the right location? (I however am a bit uncomfortable with widening a gap between two subsets of the same backends, but it doesn't mean I am going to hinder e.g. this particular thing).

but be WARNED that the built-in implementation of GLFW3 does not handle the meta key properly (does not have the workaround) and as a result will break keyboard in ImGui, so it is really not recommended to switch to macos behavior with this implementation...

Alright :(

But whether it becomes the de facto or not I don't know

My intuition is that this isn't 100% a technical problem but a political/awareness problem + conforming to whatever standard Emscripten wants. You are listing technical features - I don't disagree that your port seems better in terms of features! I am just wondering what you can do toward making this the default for all Emscripten users? Why does SDL has a sensible default and not GLFW?

We tried to make a clang-format file but there are many subtleties and custom cases.
AFAIK it seems that clang-format is not able to selectively apply only certain specific rules and leave the rest untouched? it always reformat things according to those.

@ocornut
Copy link
Owner

ocornut commented Sep 3, 2024

(FYI i am content enough that you tried to conform to the style - we all have our coding peculiarities and if it is too difficult for you I don't mean to suggest that I absolutely can't cover it myself. I'll keep fixing code anyhow, it was just an attempt to slightly reduce my cognitive load as I'm not really good at ignoring this :()

@ypujante
Copy link
Contributor Author

ypujante commented Sep 4, 2024

I'm not uncomfortable with moving forward with (4) as it is useful, my comment requested a comment and wondering if it was done in the right location? (I however am a bit uncomfortable with widening a gap between two subsets of the same backends, but it doesn't mean I am going to hinder e.g. this particular thing).

I guess I do not know the ImGui code base enough to have a strong opinion of where this piece of code should go. If you think it makes more sense to go in ImGui_ImplGlfw_InitForOther (or some other init file?) then I am good with that too. I can also add more comments to explain what the code does and why it does it. Let me know.

My intuition is that this isn't 100% a technical problem but a political/awareness problem + conforming to whatever standard Emscripten wants. You are listing technical features - I don't disagree that your port seems better in terms of features! I am just wondering what you can do toward making this the default for all Emscripten users? Why does SDL has a sensible default and not GLFW?

I do not know all the history, but what I do know is that originally there was (and still is) a built-in SDL implementation in Emscripten. One implemented 100% in JavaScript like GLFW. Again, not knowing the details of how and why it happened, there is now an official implementation of SDL (2+) that is part of the SDL project itself (example)

  • If you use -sUSE_SDL=1 then you get the built-in/JavaScript one
  • If you use -sUSE_SDL=2 (or the preferred syntax --use-port=sdl2), then you get the external port that is provided by SDL. Exactly like my port except that it is SDL hosting the files themselves.

Why is there no GLFW official support of Emscripten, that I do not know. I did ask a few questions on the GLFW forum, including reserving an ID for the Emscripten platform, so they are well aware that my port exists. I am not aware of any interest from them to embrace the platform, although to be fair I did not ask. I could certainly suggest to integrate my port with the official GLFW library, but it would probably represent a big effort as I did take a clean slate approach: implementing the API from the .h only while never looking at how they do anything. And my library is written in C++ when GLFW has no C++ that I am aware of, in other words it would be a very project to do that. And that is assuming they would be interested...

But I agree with you that I probably need to do a better job at communicating. Clearly it's a chicken and egg problem. I think that having ImGui, a world renown library, officially integrated with this port gives it a big boost. Now when people are complaining that joystick is not working, or copy/paste is not, then there is an alternative...

FWIW, I have also turned ImGui into an emscripten port as well which makes using ImGui on the Emscripten platform as easy as --use-port=imgui.py:backend=glfw:renderer=wgpu and this port uses emscripten-glfw...

I will release this separately soon...

We tried to make a clang-format file but there are many subtleties and custom cases. AFAIK it seems that clang-format is not able to selectively apply only certain specific rules and leave the rest untouched? it always reformat things according to those.

I don't enough of clang format to provide an answer unfortunately.

@ocornut ocornut changed the title Uses macOS behavior on macOS (using glfw contrib port) Emscripten: Uses macOS behavior on macOS (using glfw contrib port) Sep 4, 2024
@ocornut
Copy link
Owner

ocornut commented Sep 4, 2024

I guess I do not know the ImGui code base enough to have a strong opinion of where this piece of code should go. If you think it makes more sense to go in ImGui_ImplGlfw_InitForOther (or some other init file?) then I am good with that too.

It seems odd for it to not simply be in ImGui_ImplGlfw_Init(), should be moved there.

I can also add more comments to explain what the code does and why it does it. Let me know.

Yes.

I also suggest that we amend the top of imgui_impl_glfw.cpp with a comments:

// About Emscripten support:
// - Emscripten provides its own GLFW implementation (syntax: "xxxxx") but xxx and xxx are missing and not well supported.
// - A third-party Emscripten GLFW implementation (syntax: "xxxxx") supports xxx and xxx better.

@ypujante
Copy link
Contributor Author

ypujante commented Sep 6, 2024

I just wanted to let you know that I spent a non trivial amount of time improving the documentation of my project and adding a Comparison page so that it is easier to see the differences.

I will work on your suggested changes next for (4).

Are you planning to cherry pick (1) at some point since there is no CI for the emscripten-glfw path?

Thank you.

@ypujante
Copy link
Contributor Author

ypujante commented Sep 7, 2024

I am closing this one and created a part 2 instead PR #7965

@ypujante ypujante closed this Sep 7, 2024
ocornut pushed a commit that referenced this pull request Sep 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants