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

Fix online demo for mobile chrome #197

Merged
merged 2 commits into from
Sep 11, 2024
Merged

Fix online demo for mobile chrome #197

merged 2 commits into from
Sep 11, 2024

Conversation

wirew0rm
Copy link
Member

I was looking into why opendigitizer was not working on android chrome. I've enabled remote debugging and the error is WebGL: INVALID_VALUE: texImage2D: width or height out of range _glTexImage2D so it seems that some part of the code tries to allocate texture memory too big for mobile devices. Interestingly it works with firefox on the same device.
mrdoob/three.js#22873 describes the same symptoms, but we already have the meta tag suggested there.

Further investigation showed

  • glTexImage2D is called 3 times during the initialisation regardless of browser choice, twice with a 120x100 texture (probably the fair logo in light and dark) and once with a 4096x16384 texture
  • The big texture is allocated by imgui's font atlas, which is quite huge since we render 2 different fonts in 4 sizes each, plus a handful of fontawesome gylphs in the same sizes. Additionally we use x4 font oversampling (leading to a x16 bigger font atlas) as the fonts have to be (fractionally) scaled by the flowgraph view.
  • on my pc GLctx.getParameter(GLctx.MAX_TEXTURE_SIZE) returns 32k, on my phone it returns only 4k. This number seems to be the maximum supported texture dimension, so the height of the second one would violate that. (also that corresponds to ~270Mb ). (for comparison: the browser window is actually 432*834, so this texture fits the whole screen more than 200 times)
  • ImGUI has hardcoded limits for the font atlas size, 4k for the width and 23k for the height. It then packs the glyph rectangles into the texture to determine the actual height for the texture which will be the lowest power of two which will fit all requested glyphs
  • Still don't know why this is not a problem on mobile firefox, since this is only a warning maybe it's just that firefox uses only the valid parts of the returned texture or handles this case differently.

The Solution/fix

This PR changes the oversampling to 2, which was the easiest way to significantly reduce the font-atlas size. This has the drawback of making the fonts in the flowgraph view appear a bit blurry, depending on the zoom level, which can be improved in a later change by using the biggest font there.

Testing on 2 different android devices showed that this allows chrome on android to display the wasm app again.
The font atlas with oversampling = 1 uses a 2kx2k texture, so it was even possible to stay with x2 oversampling.

Further Work

  • use the biggest available font-size for the flowgraph view
  • In general the font handling could be improved to lower the memory footprint:
    • factor of 2 reduction by rebuilding the font atlas when switching between the 2 different themes, so it always contains only the used font
    • using a more sophisticated font caching system, which dynamically builds the font atlas for all used glyphs (or wait for ImGui to implement this themselves as they plan to, but this is not currently worked on)

Janitorial

This PR also bumps the versions of various github pages related actions, which were causing deprecation warnings and depended on an old nodejs version which will probably at some point be removed from the runners.

Rendering the webui on mobile chromium fails with logging the error
message: `INVALID_VALUE: texImage2D: width or height out of range _glTexImage2D`

After playing a bit more with remote debugging on chrome and firefox i found out a bit more:
  - glTexImage2D is called 3 times during the initialisation regardless of
  browser choice, twice with a 120x100 texture (probably the fair logo in light
  and dark) and once with a 4096x16384 texture
  - on my pc GLctx.getParameter(GLctx.MAX_TEXTURE_SIZE) returns 32k, on my phone
  it returns only 4k. This number seems to be the maximum supported texture
  dimension, so the height of the second one would violate that. (also that
  corresponds to ~270Mb ). (the browser window is actually 432*834, so this
  texture fits the whole screen more than 200 times)
  - Still don't know why this is not a problem on mobile firefox, since this is
  only a warning maybe it's just that firefox uses only the valid parts of the
  returned texture or handles this case differently.
  - we use 4 times font oversampling (to ensure that the fonts look good when
  the flowgraph is zoomed) and at the same time and all that for 4 different
  font sizes (+ some icons). I assume the flowgraph could just use the biggest
  fontsize without oversampling and be scaled down by default.

This PR tries to set the oversampling to 2, which will result in a 75%
smaller font atlas texture. Even without any oversampling there was only
a slight blur in the flowgraph view fonts, so for now 2x oversampling
should be a good compromise.

Signed-off-by: Alexander Krimm <[email protected]>
This commit updates the github pages actions to versions which use
supported nodejs versions and are generally up to date.
This fixes all the warnings in the github actions panel.

There is one change which broke the CI, as with the new versions
uploading the same artifact name twice will not overwrite but create an
error. This is fixed by only uploading the Release version.

Also allows the special branch `fixOnlineDemo` to be used to test github
pages deployment without having to merge the PR, by also publishing it
to github pages.

Signed-off-by: Alexander Krimm <[email protected]>
Copy link
Member

@RalphSteinhagen RalphSteinhagen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for following this up so meticuously and fixing it! Much appreciated!

@RalphSteinhagen RalphSteinhagen merged commit db00420 into main Sep 11, 2024
10 checks passed
@RalphSteinhagen RalphSteinhagen deleted the fixOnlineDemo branch September 11, 2024 16:05
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.

2 participants