[RFC] Initial High DPI support; fix aarch64 macOS crash on launch#3499
[RFC] Initial High DPI support; fix aarch64 macOS crash on launch#3499qwertychouskie wants to merge 9 commits intoSuperTux:masterfrom
Conversation
Tested on macOS
It's admittedly a minor annoyance, but on systems set to show hidden files by default, the `.fseventsd` folder ends up partially covering the logo.
|
I would really prefer if you kept the HIDPI changes in a separate PR, as itll he difficult for me to cherry pick out the cmake fixes here. I hate to be a bother; but its unlikely i want to mess directly with the video backend (as this commit pokes at some other areas) as i want to test it a little more thoroughly before release. Sorry :/ If you could rebase/split it into 2 that would be greatly appreciated |
|
Oh goodness i completely ignored the original message. Carry on Ill review in the morning. Tobbi will be able to test it too |
Unfortunately, no. I'm on an older Intel Mac, which doesn't enforce code signing this strictly (otherwise I'd have caught this issue earlier). Also, different architecture. |
FWIW, the commits themselves are cleanly separated, I just haven't done a final code review and spun them off to separate branches yet. |
|
Another thing I just noticed: The Apple Silicon builds require Rosetta for some reasons. Maybe you would also like to address this? |
Are you seeing a prompt to install Rosetta when launching on an Apple Silicon system? All the binaries and libraries in the app appear to be native aarch64. |
Packages that have a shell script as their primary executable are treated as x86-64 by default for compatibility reasons. Adding LSArchitecturePriority to Info.plist should make the app's detected architecture be correct, see https://apple.stackexchange.com/a/474471/167476
|
OK, I think the issue of requiring Rosetta should be fixed, though it will need to be tested by someone with an arm64 Mac without Rosetta installed to confirm. |
| install(CODE " | ||
| if(\"\$ENV{DESTDIR}\${CMAKE_INSTALL_PREFIX}/\" MATCHES \".*\\\\.app.*\") | ||
| include(BundleUtilities) | ||
| fixup_bundle(\"${APPS}\" \"\" \"${DIRS}\") | ||
| endif() | ||
| ") | ||
| if(\"\$ENV{DESTDIR}\${CMAKE_INSTALL_PREFIX}/\" MATCHES \".*\\\\.app.*\") | ||
| include(BundleUtilities) | ||
|
|
||
| # Override the default embedded path to put libraries in Contents/Frameworks | ||
| # instead of Contents/Resources/Frameworks | ||
| # The executable is at Contents/Resources/bin/supertux2, so we need ../../Frameworks | ||
| function(gp_item_default_embedded_path_override item default_embedded_path_var) | ||
| set(path \"@executable_path/../../Frameworks\") | ||
| set(\${default_embedded_path_var} \"\${path}\" PARENT_SCOPE) | ||
| endfunction() | ||
|
|
||
| fixup_bundle(\"${APPS}\" \"\" \"${DIRS}\") | ||
|
|
||
| # Clean up redundant library directory - fixup_bundle already copied them to Frameworks | ||
| file(REMOVE_RECURSE \"\$ENV{DESTDIR}\${CMAKE_INSTALL_PREFIX}/lib\") | ||
| endif() | ||
| ") |
There was a problem hiding this comment.
I would say "use a configure file for this" but it's generated CMake.............. :') Is there anything we can do here?
| set(CPACK_DEBIAN_PACKAGE_DESCRIPTION "Classic 2D jump 'n run sidescroller with Tux\n SuperTux is a jump'n'run game with strong inspiration from the Super Mario Bros. games for the various Nintendo platforms.\n \n Run and jump through multiple worlds, fighting off enemies by jumping on them, bumping them from below or tossing objects at them, grabbing power-ups and other stuff on the way.\n .\n This is a development snapshot of SuperTux. It may suffer from critical bugs and has not been fully tested. \n .\n Homepage: https://supertux.org/") | ||
| set(CPACK_DEBIAN_PACKAGE_SECTION "games") | ||
| set(CPACK_RPM_PACKAGE_NAME "supertux2") | ||
| set(CPACK_RPM_PACKAGE_DESCRIPTION "Classic 2D jump 'n run sidescroller with Tux\n SuperTux is a classic 2D jump 'n run sidescroller game in a similar\n style like the original SuperMario games. This release of SuperTux\n features 9 enemies, 26 playable levels, software and OpenGL rendering\n modes, configurable joystick and keyboard input, new music and\n completely redone graphics.\n .\n This is a development snapshot of SuperTux. It may suffer from\n critical bugs and has not been fully tested. \n .\n Homepage: http://supertux.lethargik.org/") | ||
| set(CPACK_RPM_PACKAGE_DESCRIPTION "Classic 2D jump 'n run sidescroller with Tux\n SuperTux is a jump'n'run game with strong inspiration from the Super Mario Bros. games for the various Nintendo platforms.\n \n Run and jump through multiple worlds, fighting off enemies by jumping on them, bumping them from below or tossing objects at them, grabbing power-ups and other stuff on the way.\n .\n This is a development snapshot of SuperTux. It may suffer from critical bugs and has not been fully tested. \n .\n Homepage: https://supertux.org/") |
There was a problem hiding this comment.
Really not sure why this isn't a separate variable in it's own. Have you checked the newlines btw?
There was a problem hiding this comment.
Also TBH this description is pretty lengthy. I kept the ebuild description to just "A run n' jump platforming game featuring Tux the penguin"
| flags |= SDL_WINDOW_RESIZABLE; | ||
| flags |= SDL_WINDOW_ALLOW_HIGHDPI; |
There was a problem hiding this comment.
| flags |= SDL_WINDOW_RESIZABLE; | |
| flags |= SDL_WINDOW_ALLOW_HIGHDPI; | |
| flags |= SDL_WINDOW_RESIZABLE | SDL_WINDOW_ALLOW_HIGHDPI; |
| return Vector(static_cast<float>(pixel_x - m_rect.left) / m_scale.x, | ||
| static_cast<float>(pixel_y - m_rect.top) / m_scale.y); |
There was a problem hiding this comment.
/me wonders if you could just / m_scale the vector...
| // Compute HiDPI scale factor for coordinate conversion | ||
| Size window_size = get_window_size(); | ||
| float pixel_scale = (window_size.width > 0) ? | ||
| static_cast<float>(output_w) / static_cast<float>(window_size.width) : 1.0f; |
There was a problem hiding this comment.
| static_cast<float>(output_w) / static_cast<float>(window_size.width) : 1.0f; | |
| (static_cast<float>(output_w) / static_cast<float>(window_size.width)) : 1.0f; |
Exactly.
I cherry-picked your CPack related commits and can confirm that the prompt to install Rosetta does not appear anymore. Unfortunately the game does not launch: In my local build, the supertux2 executable is placed in SuperTux.app/Contents/Resources/games/, instead of the bin directory, most likely due to In my case DISABLE_CPACK_BUNDLING is false, as I do not see that this is somehow set to true in the github workflows. |
I'm building with this: mkdir build
cd build
rm -rf * && cmake -DINSTALL_SUBDIR_BIN=bin .. && make -j16 && cpack -G BundleI assume this is being set somewhere in the CI scripts, since the CI build uses the correct directory. You can also grab the package produced by the CI run on this MR for easy testing: https://github.com/SuperTux/supertux/actions/runs/20632897867/artifacts/5001656600 |
|
Ah, correct, the following variables are passed to CMake in the CI script: With that given, SuperTux.app launches as expected on Apple Silicon (without Rosetta). From my point of view, the macOs bundle related part of this PR is working. |
|
@qwertychouskie Could you apply the review comments by @swagtoy (at least the ones that are clear) |
806ca6a to
c12b71b
Compare
Wanted to get feedback on this before doing the work to make proper separated PRs. Thoughts?