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

[GLFW] Restore css scaling behavior (removed in 3.1.51) #22900

Merged
merged 14 commits into from
Nov 15, 2024

Conversation

ypujante
Copy link
Contributor

@ypujante ypujante commented Nov 9, 2024

This is the patch I am proposing for addressing #22847

  • the changes in 3.1.51 to add HiDPI support removed a feature that is not documented nor a feature of GLFW
  • raylib wants it back so this patch restores it

Some points:

  • I added a function isCSSScalingEnabled() with documentation to make it clear what is happening
  • Due to the fact that HiDPI / 4k support overrides CSS, CSS scaling is disabled in that mode. There would be a conflict otherwise
  • Since this feature is not really standard, I introduced a way to disable it (since it was implemented prior to 3.1.51, I felt it was better to have it on by default with a way to disable it rather than the other way around).

I have tested the changes as best as I could with ImGui and raylib. I tested a few examples with raylib and it seems to be doing what they want (the canvas occupies the fullscreen and the mouse coordinates are correct and resizing the window follows appriopriately)

It would certainly be better if @michaelfiber and/or @raysan5 could test the changes before they go live. It should be a matter of checking out this PR but I don't know if they have any bandwidth to do this.

- the changes in 3.1.51 to add HiDPI support removed a feature that is not documented nor a feature of GLFW
- raylib wants it back so this patch restores it
@ypujante
Copy link
Contributor Author

ypujante commented Nov 9, 2024

After the changes are approved I will modify Changelog to add an entry about this.

@raysan5
Copy link

raysan5 commented Nov 9, 2024

@ypujante Thanks for the review. I tested this change on latest raylib master branch, with emsdk 3.1.71, replacing library_glfw.js manually, and it works as expected, the canvas and input are scaled to width: 100%.

Please, excuse me if some of my previous messages could be rude. Thanks again for reviewing it.

- in any case when adjustCanvasDimensions is called and there is no window active, there is no reason to resize the canvas
@ypujante
Copy link
Contributor Author

The only test failure I am seeing is

======================================================================
ERROR [61.246s]: test_offset_converter (test_browser.browser)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/root/project/test/common.py", line 2188, in run_browser
    self.assertContained(expected, output)
  File "/root/project/test/common.py", line 1518, in assertContained
    self.fail("Expected to find '%s' in '%s', diff:\n\n%s\n%s" % (
  File "/usr/lib/python3.8/unittest/case.py", line 753, in fail
    raise self.failureException(msg)
AssertionError: Expected to find '/report_result?exit:1
' in '[no http server activity]
', diff:

which seems to be very unlikely related to these unrelated changes

Copy link
Collaborator

@sbc100 sbc100 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 working on this fix! Much appreciated.

* It is enabled by default but can be disabled by setting Module['EMSCRIPTEN_GLFW_DISABLE_CSS_SCALING'] to true.
* It is automatically disabled when using Hi DPI (the library overrides CSS sizes). */
isCSSScalingEnabled() {
return Module['EMSCRIPTEN_GLFW_DISABLE_CSS_SCALING'] !== true && !GLFW.isHiDPIAware();
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm somewhat loath to add more incoming module API elements like this. Can we perhaps wait until somebody asks for this setting?

If we do end up adding it it would need to be added to INCOMING_MODULE_JS_API and then this line should be wrapped in #if expectToReceiveOnModule('INCOMING_MODULE_JS_API').

I simpler more pay-as-you-go option could be to add some kind of new API for this either via glfwSetWindowParam or some new C API.

I wonder if we can add a test for this behaviour? Can we perhaps make a simple reftest that shows that the GLFW windows is scaled to the size of the canvas? I'm not sure how hard that would be.

- as requested by @sbc100
- if somebody asks for a way to disable the feature we can add a C API at a later time
- although the result is the same we should be testing with the correct constant
@ypujante
Copy link
Contributor Author

FYI, I am aware of the test failure (which does not fail on my local machine). I am trying to figure out why... will report when I do.

- describe why CSS scaling cannot work in this mode
@ypujante
Copy link
Contributor Author

@sbc100 I have made the changes that you requested:

  1. I removed entirely the ability to disable CSS scaling. I suppose if we get a request to add a way to disable it, a new C API might be the right avenue. That being said, CSS scaling can be disabled simply by not setting a CSS rule for the canvas.

  2. I added a test to test the CSS scaling feature (test_glfw3_css_scaling). I took this opportunity to add documentation to this test to explain the feature. I also took this opportunity to add documentation to test_glfw3_hi_dpi_aware to document as well the HiDPI feature.

  3. The tests are all passing (including the new one of course)

  test_glfw3_hi_dpi_aware (test_browser.browser) ... cache:INFO: generating system asset: symbol_lists/244f18794cb7f6b549ab5dd0d77e3fbe5afa47ba.json... (this will be cached in "/root/cache/symbol_lists/244f18794cb7f6b549ab5dd0d77e3fbe5afa47ba.json" for subsequent builds)
cache:INFO:  - ok
...
  test_glfw3_css_scaling (test_browser.browser) ... cache:INFO: generating system asset: symbol_lists/a5a69ed40c83d2b1e0649d3494211e7bd45f85fa.json... (this will be cached in "/root/cache/symbol_lists/a5a69ed40c83d2b1e0649d3494211e7bd45f85fa.json" for subsequent builds)
cache:INFO:  - ok
  1. Besides the automated test, I did as much testing I could locally both with raylib and ImGui.

  2. If you approve of the changes, I will update the Changelog and maybe ask raylib to do a final test as Raymond tested prior to the final changes (which were required to fix HiDPI, which thankfully the automated test I wrote caught...)

@raysan5
Copy link

raysan5 commented Nov 15, 2024

Just tested with latest raylib and this PRs library_glfw.js copied over emsdk 3.1.71. It works with same previous behaviour before emsdk 3.1.51. Thank you very much for reviewing this issue so quickly.

@ypujante
Copy link
Contributor Author

@sbc100 I have made the changes you requested. I have updated the Changelog and @raysan5 verified the patch. All tests are passing. I believe this PR can be merged.

@sbc100 sbc100 merged commit 5203d08 into emscripten-core:main Nov 15, 2024
28 checks passed
@ypujante ypujante deleted the emscripten-22847 branch March 5, 2025 13:16
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.

3 participants