From c9a2752545aa2aa8b7db4879834d3998c6cac655 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vladim=C3=ADr=20Vondru=C5=A1?= Date: Sun, 3 Sep 2023 15:49:25 +0200 Subject: [PATCH] Platform: remove deprecated Emscripten event target behavior support. This makes the minimal supported Emscripten version 1.39.5. With some more effort this could be changed to 1.38.27, but I don't think anybody needs that. --- doc/changelog.dox | 3 + doc/platforms-html5.dox | 2 - src/Magnum/Platform/CMakeLists.txt | 10 ++ src/Magnum/Platform/EmscriptenApplication.cpp | 100 +++++------------- src/Magnum/Platform/EmscriptenApplication.h | 1 - src/Magnum/Platform/Sdl2Application.cpp | 19 +--- src/Magnum/Platform/Test/CMakeLists.txt | 10 +- 7 files changed, 44 insertions(+), 101 deletions(-) diff --git a/doc/changelog.dox b/doc/changelog.dox index e1fd41708..897f2b411 100644 --- a/doc/changelog.dox +++ b/doc/changelog.dox @@ -48,6 +48,9 @@ See also: - Minimal supported CMake version for Emscripten WebGL 2 compilation is now 3.13, in order to be able to set linker options for enabling WebGL 2 without having to pollute the global `CMAKE_EXE_LINKER_FLAGS` variable. +- Minimal supported Emscripten version is now 1.39.5 which implicitly sets + `-s DISABLE_DEPRECATED_FIND_EVENT_TARGET_BEHAVIOR=1`, as the deprecated + code paths were removed. @subsection changelog-latest-new New features diff --git a/doc/platforms-html5.dox b/doc/platforms-html5.dox index 50915dbe2..ae39abf2d 100644 --- a/doc/platforms-html5.dox +++ b/doc/platforms-html5.dox @@ -620,8 +620,6 @@ down the build significantly or being very recent: (new since 1.38.27) --- note that, while this leads to significant size savings, it currently requires significant amount of work on the application side and Magnum was not yet tested with this option enabled -- [`-s DISABLE_DEPRECATED_FIND_EVENT_TARGET_BEHAVIOR=1`](https://github.com/emscripten-core/emscripten/pull/7977) - (new since 1.38.27) --- leads to minor code size savings as well On the other hand, if you need shorter compile times and don't care much about code size (for example when testing on the CIs), consider *removing* some of diff --git a/src/Magnum/Platform/CMakeLists.txt b/src/Magnum/Platform/CMakeLists.txt index 0107ce246..95d3e1a53 100644 --- a/src/Magnum/Platform/CMakeLists.txt +++ b/src/Magnum/Platform/CMakeLists.txt @@ -182,6 +182,16 @@ if(MAGNUM_WITH_ANDROIDAPPLICATION) add_library(Magnum::AndroidApplication ALIAS MagnumAndroidApplication) endif() +if(CORRADE_TARGET_EMSCRIPTEN AND (MAGNUM_WITH_EMSCRIPTENAPPLICATION OR MAGNUM_WITH_SDL2APPLICATION)) + # This could also set -s DISABLE_DEPRECATED_FIND_EVENT_TARGET_BEHAVIOR=1 + # always and require only 1.38.27 (where this option got added), but that + # version is from February 2019 and I don't think anyone actively uses it + # anymore. + if(EMSCRIPTEN_VERSION VERSION_LESS 1.39.5) + message(SEND_ERROR "Emscripten 1.39.5+, which enables DISABLE_DEPRECATED_FIND_EVENT_TARGET_BEHAVIOR by default, is required to build EmscriptenApplication and Sdl2Application. Found version ${EMSCRIPTEN_VERSION}.") + endif() +endif() + # Emscripten application if(MAGNUM_WITH_EMSCRIPTENAPPLICATION) if(NOT CORRADE_TARGET_EMSCRIPTEN) diff --git a/src/Magnum/Platform/EmscriptenApplication.cpp b/src/Magnum/Platform/EmscriptenApplication.cpp index 4af13d7a9..1b42328a7 100644 --- a/src/Magnum/Platform/EmscriptenApplication.cpp +++ b/src/Magnum/Platform/EmscriptenApplication.cpp @@ -30,6 +30,7 @@ #include #include #include +#include #include #include #include @@ -42,13 +43,6 @@ #include "Magnum/GL/Version.h" #endif -/** @todo drop once we don't support < 1.38.27 anymore */ -#ifndef EMSCRIPTEN_EVENT_TARGET_DOCUMENT -#define EMSCRIPTEN_EVENT_TARGET_DOCUMENT reinterpret_cast(1) -#define EMSCRIPTEN_EVENT_TARGET_WINDOW reinterpret_cast(2) -#define EMSCRIPTEN_EVENT_TARGET_SCREEN reinterpret_cast(3) -#endif - namespace Magnum { namespace Platform { using namespace Containers::Literals; @@ -190,7 +184,7 @@ namespace { ERROR - [JSC_LANGUAGE_FEATURE] This language feature is only supported for ECMASCRIPT6 mode or better: const declaration. */ char* id = reinterpret_cast(EM_ASM_INT({ - var id = Module['canvas'].id; + var id = '#' + Module['canvas'].id; var bytes = lengthBytesUTF8(id) + 1; var memory = _malloc(bytes); stringToUTF8(id, memory, bytes); @@ -199,24 +193,6 @@ namespace { #pragma GCC diagnostic pop return Containers::String{id, [](char* data, std::size_t) { std::free(data); }}; } - - bool checkForDeprecatedEmscriptenTargetBehavior() { - /* Emscripten 1.38.27 changed to generic CSS selectors from element IDs - depending on -s DISABLE_DEPRECATED_FIND_EVENT_TARGET_BEHAVIOR=1 being - set. - https://github.com/emscripten-core/emscripten/pull/7977 - There is no simple way to check for compiler options so check - whether the new CSS selectors are being used. If so, it should find - canvas#[id] which is any canvas with the ID of Module.canvas. - The old target behavior will look for an element with id="canvas#[id]" - which could theoretically exist but that's highly unlikely. */ - bool deprecated = true; - Vector2d tempSize; - if(emscripten_get_element_css_size(("canvas#" + canvasId()).data(), &tempSize.x(), &tempSize.y()) >= 0) { - deprecated = false; - } - return deprecated; - } } EmscriptenApplication::EmscriptenApplication(const Arguments& arguments): EmscriptenApplication{arguments, Configuration{}} {} @@ -317,12 +293,7 @@ bool EmscriptenApplication::tryCreate(const Configuration& configuration) { std::ostream* verbose = _verboseLog ? Debug::output() : nullptr; - _deprecatedTargetBehavior = checkForDeprecatedEmscriptenTargetBehavior(); - if(_deprecatedTargetBehavior) { - Debug{verbose} << "Platform::EmscriptenApplication::tryCreate(): using old Emscripten target behavior"; - } - - _canvasTarget = (_deprecatedTargetBehavior ? canvasId() : "#"_s + canvasId()); + _canvasTarget = canvasId(); /* Get CSS canvas size and cache it. This is used later to detect canvas resizes in emscripten_set_resize_callback() and fire viewport events, @@ -403,16 +374,9 @@ bool EmscriptenApplication::tryCreate(const Configuration& configuration, const _devicePixelRatio = Vector2{Float(emscripten_get_device_pixel_ratio())}; Debug{verbose} << "Platform::EmscriptenApplication: device pixel ratio" << _devicePixelRatio.x(); - /* Find out which element target strings Emscripten expects. This depends on - the DISABLE_DEPRECATED_FIND_EVENT_TARGET_BEHAVIOR compiler option. */ - _deprecatedTargetBehavior = checkForDeprecatedEmscriptenTargetBehavior(); - if(_deprecatedTargetBehavior) { - Debug{verbose} << "Platform::EmscriptenApplication::tryCreate(): using old Emscripten target behavior"; - } - /* Get the canvas ID from Module.canvas, either set by EmscriptenApplication.js or overridden/manually set by the user. */ - _canvasTarget = (_deprecatedTargetBehavior ? canvasId() : "#"_s + canvasId()); + _canvasTarget = canvasId(); /* Get CSS canvas size and cache it. This is used later to detect canvas resizes in emscripten_set_resize_callback() and fire viewport events, @@ -534,7 +498,7 @@ void EmscriptenApplication::setupCallbacks(bool resizable) { changes. Better than polling for this change in every frame like Sdl2Application does, but still not ideal. */ if(resizable) { - const char* target = _deprecatedTargetBehavior ? "#window" : EMSCRIPTEN_EVENT_TARGET_WINDOW; + const char* target = EMSCRIPTEN_EVENT_TARGET_WINDOW; auto cb = [](int, const EmscriptenUiEvent* event, void* userData) -> Int { static_cast(userData)->handleCanvasResize(event); return false; /** @todo what does ignoring a resize event mean? */ @@ -559,9 +523,8 @@ void EmscriptenApplication::setupCallbacks(bool resizable) { emscripten_set_mousemove_callback(_canvasTarget.data(), this, false, ([](int, const EmscriptenMouseEvent* event, void* userData) -> Int { auto& app = *static_cast(userData); - /* With DISABLE_DEPRECATED_FIND_EVENT_TARGET_BEHAVIOR, canvasX/Y is - not initialized, so we have to rely on the target being the - canvas. That's always true for mouse events. */ + /* Relies on the target being the canvas, which should be always + true for mouse events */ Vector2i position{Int(event->targetX), Int(event->targetY)}; MouseMoveEvent e{*event, /* Avoid bogus offset at first -- report 0 when the event is @@ -586,22 +549,20 @@ void EmscriptenApplication::setupCallbacks(bool resizable) { EMSCRIPTEN_EVENT_TARGET_DOCUMENT and EMSCRIPTEN_EVENT_TARGET_WINDOW. As the lookup happens with the passed parameter and arrays support element lookup via strings, we can unify the code by returning string of - 1 or 2 if the target is document or window. This changed in Emscripten - 1.38.27 depending on -s DISABLE_DEPRECATED_FIND_EVENT_TARGET_BEHAVIOR=1 - but we don't want to force this flag on the users so the behavior - handles both. */ + 1 or 2 if the target is document or window. */ /* Note: can't use let or const, as that breaks closure compiler: ERROR - [JSC_LANGUAGE_FEATURE] This language feature is only supported for ECMASCRIPT6 mode or better: const declaration. */ - const char* keyboardListeningElement = reinterpret_cast(EM_ASM_INT({ + char* const keyboardListeningElement = reinterpret_cast(EM_ASM_INT({ var element = Module['keyboardListeningElement'] || document; if(element === document) return 1; /* EMSCRIPTEN_EVENT_TARGET_DOCUMENT */ if(element === window) return 2; /* EMSCRIPTEN_EVENT_TARGET_WINDOW */ if('id' in element) { - var bytes = lengthBytesUTF8(element.id) + 1; + var id = '#' + element.id; + var bytes = lengthBytesUTF8(id) + 1; var memory = _malloc(bytes); - stringToUTF8(element.id, memory, bytes); + stringToUTF8(id, memory, bytes); return memory; } @@ -609,20 +570,18 @@ void EmscriptenApplication::setupCallbacks(bool resizable) { })); #pragma GCC diagnostic pop - Containers::String keyboardListeningElementStorage; - if(keyboardListeningElement == EMSCRIPTEN_EVENT_TARGET_DOCUMENT) { - keyboardListeningElement = _deprecatedTargetBehavior ? "#document" : keyboardListeningElement; - } else if(keyboardListeningElement == EMSCRIPTEN_EVENT_TARGET_WINDOW) { - keyboardListeningElement = _deprecatedTargetBehavior ? "#window" : keyboardListeningElement; - } else if(keyboardListeningElement && !_deprecatedTargetBehavior) { - keyboardListeningElementStorage = "#"_s + Containers::StringView{keyboardListeningElement}; - std::free(const_cast(keyboardListeningElement)); - keyboardListeningElement = keyboardListeningElementStorage.data(); + /* If the element is a heap-allocated string, ensure it gets properly freed + after */ + Containers::ScopeGuard keyboardListeningElementStorage{NoCreate}; + if(keyboardListeningElement && + keyboardListeningElement != EMSCRIPTEN_EVENT_TARGET_DOCUMENT && + keyboardListeningElement != EMSCRIPTEN_EVENT_TARGET_WINDOW) + { + keyboardListeningElementStorage = Containers::ScopeGuard{keyboardListeningElement, std::free}; } - /* Happens only if keyboardListeningElement was set, but did not have an - `id` attribute. Instead it should be either null or undefined, a DOM - element, `window` or `document`. */ + /* Happens only if keyboardListeningElement was set, but wasn't a document + or a window and did not have an `id` attribute. */ CORRADE_ASSERT(keyboardListeningElement, "EmscriptenApplication::setupCallbacks(): invalid value for Module['keyboardListeningElement']", ); @@ -852,9 +811,8 @@ EmscriptenApplication::MouseEvent::Button EmscriptenApplication::MouseEvent::but } Vector2i EmscriptenApplication::MouseEvent::position() const { - /* With DISABLE_DEPRECATED_FIND_EVENT_TARGET_BEHAVIOR, canvasX/Y is not - initialized, so we have to rely on the target being the canvas. That's - always true for mouse events. */ + /* Relies on the target being the canvas, which should be always true for + mouse events */ return {Int(_event.targetX), Int(_event.targetY)}; } @@ -872,9 +830,8 @@ EmscriptenApplication::MouseMoveEvent::Buttons EmscriptenApplication::MouseMoveE } Vector2i EmscriptenApplication::MouseMoveEvent::position() const { - /* With DISABLE_DEPRECATED_FIND_EVENT_TARGET_BEHAVIOR, canvasX/Y is not - initialized, so we have to rely on the target being the canvas. That's - always true for mouse events. */ + /* Relies on the target being the canvas, which should be always true for + mouse events */ return {Int(_event.targetX), Int(_event.targetY)}; } @@ -901,9 +858,8 @@ Vector2 EmscriptenApplication::MouseScrollEvent::offset() const { } Vector2i EmscriptenApplication::MouseScrollEvent::position() const { - /* With DISABLE_DEPRECATED_FIND_EVENT_TARGET_BEHAVIOR, canvasX/Y is not - initialized, so we have to rely on the target being the canvas. That's - always true for mouse events. */ + /* Relies on the target being the canvas, which should be always true for + mouse events */ return {Int(_event.mouse.targetX), Int(_event.mouse.targetY)}; } diff --git a/src/Magnum/Platform/EmscriptenApplication.h b/src/Magnum/Platform/EmscriptenApplication.h index 521fffdf7..460bda981 100644 --- a/src/Magnum/Platform/EmscriptenApplication.h +++ b/src/Magnum/Platform/EmscriptenApplication.h @@ -908,7 +908,6 @@ class EmscriptenApplication { Flags _flags; Cursor _cursor = Cursor::Arrow; - bool _deprecatedTargetBehavior{}; Containers::String _canvasTarget; #ifdef MAGNUM_TARGET_GL diff --git a/src/Magnum/Platform/Sdl2Application.cpp b/src/Magnum/Platform/Sdl2Application.cpp index cab1cc188..1eff0e93c 100644 --- a/src/Magnum/Platform/Sdl2Application.cpp +++ b/src/Magnum/Platform/Sdl2Application.cpp @@ -414,13 +414,6 @@ bool Sdl2Application::tryCreate(const Configuration& configuration) { /** @todo don't hardcode "#canvas" here, make it configurable from outside */ { Vector2d canvasSize; - /* Emscripten 1.38.27 changed to generic CSS selectors from element - IDs depending on -s DISABLE_DEPRECATED_FIND_EVENT_TARGET_BEHAVIOR=1 - being set (which we can't detect at compile time). Fortunately, - using #canvas works the same way both in the previous versions and - the current one. Unfortunately, this is also the only value that - works the same way for both. Further details at - https://github.com/emscripten-core/emscripten/pull/7977 */ emscripten_get_element_css_size("#canvas", &canvasSize.x(), &canvasSize.y()); _lastKnownCanvasSize = Vector2i{canvasSize}; } @@ -650,13 +643,6 @@ bool Sdl2Application::tryCreate(const Configuration& configuration, const GLConf /** @todo don't hardcode "#canvas" here, make it configurable from outside */ { Vector2d canvasSize; - /* Emscripten 1.38.27 changed to generic CSS selectors from element - IDs depending on -s DISABLE_DEPRECATED_FIND_EVENT_TARGET_BEHAVIOR=1 - being set (which we can't detect at compile time). Fortunately, - using #canvas works the same way both in the previous versions and - the current one. Unfortunately, this is also the only value that - works the same way for both. Further details at - https://github.com/emscripten-core/emscripten/pull/7977 */ emscripten_get_element_css_size("#canvas", &canvasSize.x(), &canvasSize.y()); _lastKnownCanvasSize = Vector2i{canvasSize}; } @@ -884,12 +870,9 @@ bool Sdl2Application::mainLoopIteration() { size here. But only if the window was requested to be resizable, to avoid resizing the canvas when the user doesn't want that. Related issue: https://github.com/kripken/emscripten/issues/1731 */ + /** @todo don't hardcode "#canvas" here, make it configurable from outside */ if(_flags & Flag::Resizable) { Vector2d canvasSize; - /* Emscripten 1.38.27 changed to generic CSS selectors from element - IDs depending on -s DISABLE_DEPRECATED_FIND_EVENT_TARGET_BEHAVIOR=1 - being set (which we can't detect at compile time). See above for the - reason why we hardcode #canvas here. */ emscripten_get_element_css_size("#canvas", &canvasSize.x(), &canvasSize.y()); const Vector2i canvasSizei{canvasSize}; diff --git a/src/Magnum/Platform/Test/CMakeLists.txt b/src/Magnum/Platform/Test/CMakeLists.txt index c1f55d19d..9e7cd7e75 100644 --- a/src/Magnum/Platform/Test/CMakeLists.txt +++ b/src/Magnum/Platform/Test/CMakeLists.txt @@ -50,11 +50,8 @@ if(MAGNUM_WITH_EMSCRIPTENAPPLICATION) add_executable(PlatformEmscriptenApplicationTest EmscriptenApplicationTest.cpp) target_link_libraries(PlatformEmscriptenApplicationTest PRIVATE MagnumEmscriptenApplication MagnumGL) + # Enable memory runtime checks target_link_options(PlatformEmscriptenApplicationTest PRIVATE - # Test that the canvas and keylistener can be found with both the old - # and the new Emscripten target behaviors - "SHELL:-s DISABLE_DEPRECATED_FIND_EVENT_TARGET_BEHAVIOR=0" - # Enable memory runtime checks "SHELL:-s SAFE_HEAP=1") # TODO Emscripten's own event handling generates float-to-int conversion # errors with ASSERTIONS=2, using ASSERTIONS=1 until fixed: @@ -78,11 +75,8 @@ if(MAGNUM_WITH_EMSCRIPTENAPPLICATION) add_executable(PlatformMultipleEmscriptenApplicationTest EmscriptenApplicationTest.cpp) target_link_libraries(PlatformMultipleEmscriptenApplicationTest PRIVATE MagnumEmscriptenApplication MagnumGL) + # Enable memory runtime checks target_link_options(PlatformMultipleEmscriptenApplicationTest PRIVATE - # Test that the canvas and keylistener can be found with both the old - # and the new Emscripten target behaviors - "SHELL:-s DISABLE_DEPRECATED_FIND_EVENT_TARGET_BEHAVIOR=1" - # Enable memory runtime checks "SHELL:-s SAFE_HEAP=1") # TODO Emscripten's own event handling generates float-to-int conversion # errors with ASSERTIONS=2, using ASSERTIONS=1 until fixed: