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

Enable 4k/retina support (example_emscripten_wgpu) #7151

Closed
wants to merge 2 commits into from

Conversation

ypujante
Copy link
Contributor

Hi @ocornut

Following the merge you did this morning for emscripten/glfw, I am pushing a new pull request to enable 4k/retina support in the example_emscripten_wgpu example.

Background:
I am the author of the changes that got merged in emscripten/glfw recently to enable support for 4k/retina displays, thus making the rendering much higher quality. Here are the release notes (that I wrote) from emscripten 3.1.51:

Added Hi DPI support to GLFW. When enabled, GLFW automatically accounts for the devicePixelRatio browser property and changes the size of the canvas accordingly (including dynamically if the canvas is moved from a 4K screen to a 2K screen and vice-versa). glfwGetFramebufferSize now properly returns the canvas size in pixels, while glfwGetWindowSize returns the canvas size is screen size. By default, this feature is disabled. You can enable it before creating a window by calling glfwWindowHint(GLFW_SCALE_TO_MONITOR, GLFW_TRUE). You can also dynamically change it after the window has been created by calling glfwSetWindowAttrib(window, GLFW_SCALE_TO_MONITOR, GLFW_TRUE). (#20584)

As you can see from this pull request, the changes are very minimal. Also, because emscripten/glfw prior to version 3.1.51 was ignoring the glfw window hint GLFW_SCALE_TO_MONITOR, then having this hint present will not affect any user not using the latest version of 3.1.51

In terms of result I took some screenshot so you can see a before/after comparison:

Low resolution:
imgui-2k
custom-rendering-2k

High resolution:
imgui-4k
custom-rendering-4k

As you can see it is particularly noticeable on everything that is drawn, like the checkboxes, the triangle to close/open the window or any shape in the custom rendering example.

In terms of changes:

  1. Added the hint to enable hi dpi awareness (if the screen is not 4k or if you move the window from a 2k screen to 4k screen, it automatically does the right thing) glfwWindowHint(GLFW_SCALE_TO_MONITOR, GLFW_TRUE)
  2. Surrounded the canvas with a div that is the one managing the width/height (glfw automatically forces the canvas to be 2x the size (frame buffer => glfwGetFramebufferSize) and uses css to render it at screen size (glfwGetWindowSize) and use this div for ImGui_ImplGlfw_InstallEmscriptenCanvasResizeCallback.
  3. Added a css property to disable the blue border that gets rendered by the browser when the canvas gets the focus (I have seen this on occasion and it looks quite ugly)

I am going to push a live example of this changes once this PR is created.

ypujante added a commit to pongasoft/pongasoft.github.io that referenced this pull request Dec 19, 2023
@ocornut
Copy link
Owner

ocornut commented Dec 19, 2023

Hello,
Following the conversation from #6751 i would appreciate if we could make the two html templates converge into 1 used by all 3 examples, rather than have them drift further apart.
The use of a new Emscripten feature could also be #if conditioned by EMSCRIPTEN compile-time version, as a way of documenting the feature newness.

@ypujante
Copy link
Contributor Author

Ok no problem. I will look into it.

@ypujante
Copy link
Contributor Author

The demo is live

Note that you must use Chrome as Firefox currently does not support webgpu (nothing to do with my changes). You need to use a 4k monitor to see the difference...

@ypujante
Copy link
Contributor Author

After struggling for a while I was able to consolidate in a single html file example/libs/shell-minimal.html.

  • I use conditional (#if USE_WEBGPU) so that the block of code is not included for opengl3
  • I use async loading because it is required for when webgpu is required
  • I added the <div id="canvas-container"> as it is required for glfw but does not affect opengl

I have tested the following:

  • example_emscripten_wgpu => now uses example/libs/shell-minimal.html instead of its own
  • example_glfw_opengl3 => which now also renders in 4k (I made the same code change I did in example_emscripten_wgpu)
  • example_sdl2_opengl3 => which still works with the changes in example/libs/shell-minimal.html (and was rendering 4k without any change in the first place)
  • example_sdl3_opengl3 => I cannot compile on my machine as I don't have SDL3 (for some reason SDL2 gets pulled in automatically by emscripten, but that does not seem to be the case with SDL3...). I am actually not sure if anybody has ever tested this example as the Makefile uses USE_SDL=2 so I am not sure how that can ever work

@ocornut
Copy link
Owner

ocornut commented Dec 19, 2023

SDL3 + Emscripten is untested, i must have left a makefile to facilitate testing once it becomes avaialble but its probably not going to be soon.

@ypujante
Copy link
Contributor Author

@ocornut I may have found a way to get rid of the <div> I added in the html (after looking at the SDL code and why it works without the div, I think I understand what they do...) but it would require a change in emscripten. Let me investigate before you merge this and I will report back soon.

@ocornut
Copy link
Owner

ocornut commented Dec 19, 2023

Err its fine to put whatever we need in the HTML, and seems overkill to alter Emscripten.

My request was that we avoid multiple HTML files unless there is a good reason for them to be different, and if they are different that we facilitate synching of them by reducing their diff.

It’s also nice that our template HTML stays somehow aligned with common standards eg Emscripten owns default template.

@ypujante
Copy link
Contributor Author

ypujante commented Dec 20, 2023

It's actually not a big problem to update emscripten. I already submitted the PR. I just realized that what SDL was doing was pretty clever and as a result they didn't need this extra div. And so changing glfw in emscripten benefits everybody, not just this PR...

It will also make the ImGui HTML template closer to the one in emscripten (since that one does not use the div)

Now I would prefer to wait until the emscripten PR is merged/released to proceed with this one because it will be cleaner. Do you want me to close it and reopen one later? Or simply keep it open until then?

@ocornut
Copy link
Owner

ocornut commented Dec 20, 2023

We can keep open until a release happens.

@Zelif
Copy link

Zelif commented Dec 25, 2023

@ypujante
I think we can avoid having any webgpu code in any of the html files. In the main.cpp in the example if you get the instance and require the adapter and device this will mean all webgpu related code will be contained in the cpp file.

I'm yet to fix my PR up to make this cleaner but when acquiring all the needed items I removed the js code that inits the instance adapter and device in the html and it still is happy. I have yet to remove it on the branch yet but tested it locally if you want to give it a go and test you can see the changes I have done here:

https://github.com/ocornut/imgui/pull/7132/files#diff-035d873b58d64bd3f4b448039fb94dd3d56492d1ce8c2c5ed59e42946cd22e1aR174

Is there any issue you can foresee (maybe a browser I'll have to check) that calling all the init webgpu code from the cpp will cause ?
I'll be able to work on and test it once I get back home after all the christmas dealings.

@ypujante
Copy link
Contributor Author

@Zelif Thank you for participating in the discussion and providing your feedback.

IMO, I think that you are correct that it would be desirable to have a single html file that is free of webgpu code entirely. To be honest I did try this approach but my "naive" implementation did not work.

What I tried was to take the webgpu code from html and put it in main.cpp like this:

int main(int, char**)
{
  EM_ASM({
            const adapter = await navigator.gpu.requestAdapter();
            const device = await adapter.requestDevice();
            Module.preinitializedWebGPUDevice = device;
  });
// ....

But unfortunately that did not work due to await which was throwing an error at runtime (I do not remember the error message exactly), even with -sASYNCIFY option.

This is why I reverted to putting it back in the html with conditional compilation.

It sounds like you have found a way to make it work but it is just hard for me to see how exactly in the diff you pointed to (as a side note, I am not sure I understand why you have conditional compilation on __EMSCRIPTEN__ since this is a webgpu exclusive main.cpp code and as such can only be compiled with webgpu, but not knowing the details, I may be missing something).

So to answer your question, if it works in the cpp code, I don't think I can foresee an issue and I believe it to be cleaner overall. The less html/javascript code, the better. I suppose this PR can be merged first the way it is and then you can simply move the code to cpp? I am not sure what @ocornut would prefer.

Finally, I can see that your PR is about multiviewport support. I am not sure if that can be of any help, but I am currently working on a brand new glfw emscripten port from the ground up. It is implemented in cpp and it supports natively multiple glfw windows (meaning you can call glfwCreateWindow multiple times, each "window" being associated to a different canvas). It is not released yet but wanted to put in on your radar if that can help.

@Zelif
Copy link

Zelif commented Dec 26, 2023

@ypujante
I have been looking at the work that you have been doing as it intersects with multi viewport stuff I have been implementing for wgpu on desktop and i saw you mention multi canvas inside the browser (will be awesome!).

The conditionals are in there as I am in the process of changing the example for webgpu to support both web and desktop there were not many changes but one of them resulted in getting the instance, device and adapter within the cpp code. Main differences are:

  • How the main loop is registered and called. (1)
  • Creation of surface from window or canvas. (2)
  • Swap to async acquisition of instance, adapter, device (3)

Mainloop (1)

#if defined(__EMSCRIPTEN__)
       emscripten_set_main_loop_arg(MainLoopStep, window, 0, false);
#else
       while (!glfwWindowShouldClose(window)) {
           MainLoopStep(window);
           wgpuSwapChainPresent(wgpu_swap_chain);
           if (ImGui::GetIO().ConfigFlags & ImGuiConfigFlags_ViewportsEnable)
           {
               ImGui::UpdatePlatformWindows();
               ImGui::RenderPlatformWindowsDefault();
           }
       }
#endif

Surface Creation (2)

#if defined(__EMSCRIPTEN__)
    wgpu::SurfaceDescriptorFromCanvasHTMLSelector html_surface_desc = {};
    html_surface_desc.selector = "#canvas";

    wgpu::SurfaceDescriptor surfaceDesc = {};
    surfaceDesc.nextInChain = &html_surface_desc;

    wgpu_surface = wgpu_instance.CreateSurface(&surfaceDesc);
#else
    wgpu_surface = wgpu::glfw::CreateSurfaceForWindow(wgpu_instance, window);
#endif

Async loading inside main.cpp (3)

General idea is that we need to grab the instance, adapter and device async via callbacks:

static void RequestDeviceCallback(WGPURequestDeviceStatus status, WGPUDevice cDevice, const char* message, void* userdata)
{
   wgpu::Device device = wgpu::Device::Acquire(cDevice);
   wgpuDeviceSetUncapturedErrorCallback(cDevice, print_wgpu_error, nullptr);
   reinterpret_cast<void (*)(wgpu::Device)>(userdata)(device);
}

static void RequestAdapterCallback(WGPURequestAdapterStatus status, WGPUAdapter cAdapter, const char* message, void* userdata)
{
   if (status != WGPURequestAdapterStatus_Success) {
       exit(0);
   }

   wgpu::Adapter adapter = wgpu::Adapter::Acquire(cAdapter);

   adapter.RequestDevice(nullptr, RequestDeviceCallback, userdata);
}

static void InitWGPU(void (*callback)(wgpu::Device))
{
   wgpu_instance = wgpu::Instance::Acquire(wgpuCreateInstance(nullptr));
   wgpu_instance.RequestAdapter(nullptr, RequestAdapterCallback, reinterpret_cast<void*>(callback));
}

InitWGPU([](wgpu::Device device) {
   // Got device, adapter, instance
   // continue with loading like normal
}

I want to clean this up a bit to reduce the changes and harder to look at callback (am unsure how but that is something to look at later :D ). Hopefully it helps out, this does use the #include <webgpu/webgpu_cpp.h> header and changed a few thing over to some of those wrapper functions (some were already being used).

Edit: And yeah if you merge it in the way it is, that is fine I will revert when the multiviewport work is ready to be pushed out. Was interjecting for the most part as something I can remember to do and find easier.

@ypujante
Copy link
Contributor Author

@Zelif Thank you for your explanation. I guess I am not familiar with webgpu in general, but I guess I don't understand what is webgpu for desktop. Isn't webgpu for the web by definition? I am just very curious.

@Zelif
Copy link

Zelif commented Dec 26, 2023

@ypujante
I am not very much either I am still learning it, even tho by name it is web really it is just a spec that can be coded against.
Both Firefox and Chrome have their implementations open to use for desktop programs. I like to think of it more akin to openGL but using the more modern backends (vulkan, metal and Dx12).

Just unfortunately named :D

This was me running both in web and desktop:
https://github.com/ocornut/imgui/assets/666945/ae11dc3e-9596-4a22-bccc-c06e53985cac

@ypujante
Copy link
Contributor Author

ypujante commented Dec 26, 2023

@Zelif

I have been looking at the work that you have been doing as it intersects with multi viewport stuff I have been implementing for wgpu on desktop and i saw you mention multi canvas inside the browser (will be awesome!).

I just released the wip project. Still a lot to do, but it's working (and with ImGui/example_emscripten_gpu no less)

@ypujante
Copy link
Contributor Author

ypujante commented Jan 3, 2024

Hi @ocornut, Happy New year.

I have reverted the PR that I had ongoing with emscripten because it introduced some side effects that I had not anticipated and there is no "quick" fix.

As a result, this PR, in its current state, is in my opinion the right way to go for ImGui to add 4k support for the emscripten/glfw backend. I can see that there are currently no conflicts to merge this branch, but please let me know if you want me to merge the latest base branch to this PR before you move forward.

Thank you

@ypujante
Copy link
Contributor Author

Given that it has been a while and now there are conflicts for merging, I am closing this PR for now. I will have a better solution soon once my new project which has now reached 1.0 is integrated in emscripten. I will open a new PR at a later time.

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.

3 participants