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

Remove SDL2 dependencies: Phase 1 #5458

Merged
merged 76 commits into from
May 31, 2017
Merged

Remove SDL2 dependencies: Phase 1 #5458

merged 76 commits into from
May 31, 2017

Conversation

IntelOrca
Copy link
Contributor

No description provided.

@AaronVanGeffen
Copy link
Member

Just to clarify: is the goal to get rid of SDL altogether, or just separate it from the logic to allow for GUI-less builds? I initially thought the latter, but the openrct2-ui tree and its platform-specific window contexts make me think the former is the case.

@IntelOrca
Copy link
Contributor Author

@AaronVanGeffen SDL2 will still be used, we are just trying to extract the logic so that we can build a stand alone CLI version for servers.

The UI context stuff has been designed with SDL2 to be somewhat interchangeable - just so that porting can be done to a platform that SDL2 does not support.

@@ -0,0 +1,30 @@
#pragma region Copyright (c) 2014-2016 OpenRCT2 Developers
Copy link
Contributor

Choose a reason for hiding this comment

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

2017

"${CMAKE_CURRENT_LIST_DIR}/*.hpp")

# Outputs
set (PROJECT openrct2)
Copy link
Contributor

Choose a reason for hiding this comment

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

why the space?


namespace Cursors
namespace OpenRCT2 { namespace Ui
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this supposed to be on a single line?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I intend on changing it to OpenRCT2::Ui when we can use C++17.


std::string ShowFileDialog(SDL_Window * window, const FileDialogDesc &desc) override
{
@autoreleasepool
Copy link
Contributor

Choose a reason for hiding this comment

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

File uses tabs for indenting

# -Wstrict-overflow is only active when -fstrict-overflow is enabled, but -fstrict-overflow
# is enabled on -O2, -O3, -Os. This should help catch bugs locally before they reach Travis
# As of 2a435bf -Wstrict-overflow=1 passes, but higher values do not.
set(COMMON_COMPILE_OPTIONS "${COMMON_COMPILE_OPTIONS} -fstrict-overflow")
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't COMMON_COMPILE OPTIONS be added to the c/cxx flags?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe, this was copied from the current one, so if you can improve it - by all means.

@marijnvdwerf
Copy link
Contributor

Trying to get Cmake to work on macOS:

[ 93%] Linking CXX shared library libopenrct2.dylib
Undefined symbols for architecture x86_64:
  "_EVP_DigestSignFinal", referenced from:
      NetworkKey::Sign(unsigned char const*, unsigned long, char**, unsigned long*) in NetworkKey.cpp.o
  "_EVP_DigestSignInit", referenced from:
      NetworkKey::Sign(unsigned char const*, unsigned long, char**, unsigned long*) in NetworkKey.cpp.o
  "_EVP_DigestVerifyFinal", referenced from:
      NetworkKey::Verify(unsigned char const*, unsigned long, char const*, unsigned long) in NetworkKey.cpp.o
  "_EVP_DigestVerifyInit", referenced from:
      NetworkKey::Verify(unsigned char const*, unsigned long, char const*, unsigned long) in NetworkKey.cpp.o
  "_EVP_PKEY_CTX_ctrl", referenced from:
      NetworkKey::Generate() in NetworkKey.cpp.o
  "_EVP_PKEY_CTX_free", referenced from:
      NetworkKey::~NetworkKey() in NetworkKey.cpp.o
  "_EVP_PKEY_CTX_new_id", referenced from:
      NetworkKey::NetworkKey() in NetworkKey.cpp.o
  "_EVP_PKEY_keygen", referenced from:
      NetworkKey::Generate() in NetworkKey.cpp.o
  "_EVP_PKEY_keygen_init", referenced from:
      NetworkKey::Generate() in NetworkKey.cpp.o
  "_iconv", referenced from:
      _win1252_to_utf8 in localisation.c.o
  "_iconv_close", referenced from:
      _win1252_to_utf8 in localisation.c.o
  "_iconv_open", referenced from:
      _win1252_to_utf8 in localisation.c.o
ld: symbol(s) not found for architecture x86_64

@marijnvdwerf
Copy link
Contributor

Also:

CMake Warning (dev):
  Policy CMP0042 is not set: MACOSX_RPATH is enabled by default.  Run "cmake
  --help-policy CMP0042" for policy details.  Use the cmake_policy command to
  set the policy and suppress this warning.

  MACOSX_RPATH is not specified for the following targets:

   libopenrct2

This warning is for project developers.  Use -Wno-dev to suppress it.

// _lastKeyPressed = e.key.keysym.sym;
// _keysPressed[e.key.keysym.scancode] = 1;

GetContext()->GetUiContext()->SetKeysPressed(key, e->key.keysym.scancode);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tabs!

Copy link
Contributor

Choose a reason for hiding this comment

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

Already fixed in 961a4dd.

{
_lastKeyPressed = keysym;
_keysPressed[scancode] = 1;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tabs!

@@ -119,6 +119,7 @@ namespace OpenRCT2
virtual void SetCursorTrap(bool value) abstract;
virtual const uint8 * GetKeysState() abstract;
virtual const uint8 * GetKeysPressed() abstract;
virtual void SetKeysPressed(uint32 keysym, uint8 scancode) abstract;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tabs!

@duncanspumpkin
Copy link
Contributor

@rwjuk there is a plugin for visual studio that warns you if you try to save a file with mixed tabs and spaces. It's very useful for this project.

@Gymnasiast
Copy link
Member

@duncanspumpkin Richard uses macOS and Linux, so a VS plug-in is not an option.

@pizza2004
Copy link
Contributor

Actually, Visual Studio just came out on macOS, but I wouldn't personally use it anyway.

@IntelOrca
Copy link
Contributor Author

@pizza2004 Visual Studio for Mac is very much a different application entirely so you can't use any extensions for VS in VS for Mac.

@marijnvdwerf
Copy link
Contributor

What about a git commit hook script?

@pizza2004
Copy link
Contributor

@IntelOrca Obviously, I just thought it was an interesting thing people might like to know.

@rwjuk rwjuk force-pushed the refactor/nosdl/phase-1b branch 2 times, most recently from 855d9fe to 0fe6667 Compare May 19, 2017 17:56
@IntelOrca IntelOrca force-pushed the refactor/nosdl/phase-1b branch 4 times, most recently from ced727b to ef225a6 Compare May 21, 2017 00:08
@Gymnasiast
Copy link
Member

Gymnasiast commented May 22, 2017

If I attempt to load a save using the native dialog, no saves are listed. There is also no option to select 'All files' (there used to be). Furthermore, if I click 'Cancel', it will display an error message and force me to select a file, giving me no option to cancel other than forcibly killing the OpenRCT2 process.

Edit: this is all on Ubuntu 17.04.

@marijnvdwerf
Copy link
Contributor

Fails at the linking step for me

[361/361] Linking CXX executable openrct2
FAILED: openrct2 
: && /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/c++  -std=gnu++14  -Wno-error=deprecated-declarations -Wno-error=objc-method-access -fstrict-aliasing -Werror -Wundef -Wmissing-declarations -Winit-self -Wall -Wno-unknown-pragmas -Wno-unused-function -Wno-missing-braces  -Wno-comment -Wshadow  -Wmissing-declarations -Wnonnull -DDEBUG=0 -fPIC  -Wno-error=date-time -Wnull-dereference -Wredundant-decls -Wignored-qualifiers -Wstrict-overflow=1 -std=gnu++14 -Wl,-search_paths_first -Wl,-headerpad_max_install_names  CMakeFiles/openrct2.dir/src/openrct2-ui/CursorData.cpp.o CMakeFiles/openrct2.dir/src/openrct2-ui/CursorRepository.cpp.o CMakeFiles/openrct2.dir/src/openrct2-ui/TextComposition.cpp.o CMakeFiles/openrct2.dir/src/openrct2-ui/Ui.cpp.o CMakeFiles/openrct2.dir/src/openrct2-ui/UiContext.Linux.cpp.o CMakeFiles/openrct2.dir/src/openrct2-ui/UiContext.Win32.cpp.o CMakeFiles/openrct2.dir/src/openrct2-ui/UiContext.cpp.o CMakeFiles/openrct2.dir/src/openrct2-ui/audio/AudioChannel.cpp.o CMakeFiles/openrct2.dir/src/openrct2-ui/audio/AudioContext.cpp.o CMakeFiles/openrct2.dir/src/openrct2-ui/audio/AudioMixer.cpp.o CMakeFiles/openrct2.dir/src/openrct2-ui/audio/FileAudioSource.cpp.o CMakeFiles/openrct2.dir/src/openrct2-ui/audio/MemoryAudioSource.cpp.o CMakeFiles/openrct2.dir/src/openrct2-ui/drawing/engines/SoftwareDrawingEngine.cpp.o CMakeFiles/openrct2.dir/src/openrct2-ui/drawing/engines/opengl/CopyFramebufferShader.cpp.o CMakeFiles/openrct2.dir/src/openrct2-ui/drawing/engines/opengl/DrawImageShader.cpp.o CMakeFiles/openrct2.dir/src/openrct2-ui/drawing/engines/opengl/DrawLineShader.cpp.o CMakeFiles/openrct2.dir/src/openrct2-ui/drawing/engines/opengl/FillRectShader.cpp.o CMakeFiles/openrct2.dir/src/openrct2-ui/drawing/engines/opengl/OpenGLAPI.cpp.o CMakeFiles/openrct2.dir/src/openrct2-ui/drawing/engines/opengl/OpenGLDrawingEngine.cpp.o CMakeFiles/openrct2.dir/src/openrct2-ui/drawing/engines/opengl/OpenGLFramebuffer.cpp.o CMakeFiles/openrct2.dir/src/openrct2-ui/drawing/engines/opengl/OpenGLShaderProgram.cpp.o CMakeFiles/openrct2.dir/src/openrct2-ui/drawing/engines/opengl/SwapFramebuffer.cpp.o CMakeFiles/openrct2.dir/src/openrct2-ui/drawing/engines/opengl/TextureCache.cpp.o CMakeFiles/openrct2.dir/src/openrct2-ui/UiContext.macOS.mm.o  -o openrct2  libopenrct2.dylib -lSDL2 -lSDL2_ttf -lSDL2 -framework OpenGL -ljansson -lspeexdsp -lpng16 -lz -lzip -lz -lzip -ldl -lcurl /usr/local/Cellar/openssl/1.0.2k/lib/libssl.dylib /usr/local/Cellar/openssl/1.0.2k/lib/libcrypto.dylib -lSDL2 -lSDL2_ttf -lSDL2 -lSDL2_ttf /usr/lib/libiconv.dylib && :
Undefined symbols for architecture x86_64:
  "_OBJC_CLASS_$_NSAlert", referenced from:
      objc-class-ref in UiContext.macOS.mm.o
  "_OBJC_CLASS_$_NSMutableArray", referenced from:
      objc-class-ref in UiContext.macOS.mm.o
  "_OBJC_CLASS_$_NSOpenPanel", referenced from:
      objc-class-ref in UiContext.macOS.mm.o
  "_OBJC_CLASS_$_NSSavePanel", referenced from:
      objc-class-ref in UiContext.macOS.mm.o
  "_OBJC_CLASS_$_NSString", referenced from:
      objc-class-ref in UiContext.macOS.mm.o
  "_OBJC_CLASS_$_NSURL", referenced from:
      objc-class-ref in UiContext.macOS.mm.o
  "_OBJC_CLASS_$_NSWindow", referenced from:
      objc-class-ref in UiContext.macOS.mm.o
  "___CFConstantStringClassReference", referenced from:
      CFString in UiContext.macOS.mm.o
      CFString in UiContext.macOS.mm.o
      CFString in UiContext.macOS.mm.o
      CFString in UiContext.macOS.mm.o
      CFString in UiContext.macOS.mm.o
  "_objc_autoreleasePoolPop", referenced from:
      OpenRCT2::Ui::macOSContext::macOSContext() in UiContext.macOS.mm.o
      OpenRCT2::Ui::macOSContext::ShowMessageBox(SDL_Window*, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&) in UiContext.macOS.mm.o
      OpenRCT2::Ui::macOSContext::ShowFileDialog(SDL_Window*, OpenRCT2::Ui::FileDialogDesc const&) in UiContext.macOS.mm.o
      OpenRCT2::Ui::macOSContext::ShowDirectoryDialog(SDL_Window*, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&) in UiContext.macOS.mm.o
  "_objc_autoreleasePoolPush", referenced from:
      OpenRCT2::Ui::macOSContext::macOSContext() in UiContext.macOS.mm.o
      OpenRCT2::Ui::macOSContext::ShowMessageBox(SDL_Window*, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&) in UiContext.macOS.mm.o
      OpenRCT2::Ui::macOSContext::ShowFileDialog(SDL_Window*, OpenRCT2::Ui::FileDialogDesc const&) in UiContext.macOS.mm.o
      OpenRCT2::Ui::macOSContext::ShowDirectoryDialog(SDL_Window*, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&) in UiContext.macOS.mm.o
  "_objc_msgSend", referenced from:
      OpenRCT2::Ui::macOSContext::macOSContext() in UiContext.macOS.mm.o
      OpenRCT2::Ui::macOSContext::ShowMessageBox(SDL_Window*, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&) in UiContext.macOS.mm.o
      OpenRCT2::Ui::macOSContext::ShowFileDialog(SDL_Window*, OpenRCT2::Ui::FileDialogDesc const&) in UiContext.macOS.mm.o
      OpenRCT2::Ui::macOSContext::ShowDirectoryDialog(SDL_Window*, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&) in UiContext.macOS.mm.o
ld: symbol(s) not found for architecture x86_64
clang: error: linker command failed with exit code 1 (use -v to see invocation)
ninja: build stopped: subcommand failed.

@rwjuk
Copy link
Contributor

rwjuk commented May 22, 2017

@Gymnasiast What OS was this on?

@rwjuk
Copy link
Contributor

rwjuk commented May 23, 2017

Tested and working on macOS 10.12

Copy link
Member

@janisozaur janisozaur left a comment

Choose a reason for hiding this comment

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

I haven't tested it thoroughly, but the features I want for now seem to be working. To not hold it off any longer I'm happy to have it merged as-is, with fixes, if any, merged in develop.

@janisozaur
Copy link
Member

@grahamedgecombe if you're around, can you please also take a look at this? We want to have libopenrct2.so and its clients: openrct2 (GUI) and openrct2-cli (headless build), how would we best address the lookup of libopenrct2.so for portable builds? It may also affect packaging, which I haven't inspected.

@grahamedgecombe
Copy link
Contributor

@janisozaur for portable builds set an rpath with $ORIGIN maybe?

@Gymnasiast
Copy link
Member

With this last commit it appears to work correctly on my system. Moving both the binary and lib from the build dir to the executable dir now works as intended.

CMakeLists.txt Outdated
if (PORTABLE)
install(TARGETS "libopenrct2" LIBRARY DESTINATION "bin")
else ()
install(TARGETS "libopenrct2" LIBRARY DESTINATION "usr/local/lib")
Copy link
Contributor

Choose a reason for hiding this comment

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

The DESTINATION here should just be "lib". cmake already adds the prefix, which may not be /usr/local - e.g. it's /usr in the Arch package.

Currently this results in libopenrct2.so being installed in /usr/usr/local/lib.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So you think it should be lib rather than local/lib?

Copy link
Contributor

@grahamedgecombe grahamedgecombe May 31, 2017

Choose a reason for hiding this comment

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

Yeah, I think it should be install(TARGETS "libopenrct2" LIBRARY DESTINATION "lib").

cmake adds the /usr or /usr/local (or whatever) prefix for you - based on the value of CMAKE_INSTALL_PREFIX.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I have changed it back to lib.

Which set rpath to $ORIGIN so it can be run out the box.
@@ -75,6 +75,11 @@ if (WIN32)
# tell it that it is
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -D__USE_MINGW_ANSI_STDIO=1")
endif ()

if (PORTABLE)
set(CMAKE_INSTALL_RPATH "$ORIGIN")
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't this get interpreted as "fill with contents of variable named 'ORIGIN'?

Copy link
Contributor Author

@IntelOrca IntelOrca May 31, 2017

Choose a reason for hiding this comment

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

CMAKE uses ${...} for variables and $ENV{...} for environment variables. I think this is saw on various internet pages for how to do it in cmake. It works when I build it myself.

Copy link
Contributor

Choose a reason for hiding this comment

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

set(CMAKE_INSTALL_RPATH "$ORIGIN") needs to go before add_executable() for it to work I think.

Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't the other frontend also receive the same treatment?

Copy link
Contributor

Choose a reason for hiding this comment

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

@janisozaur Derp. Yes, set(CMAKE_INSTALL_RPATH "$ORIGIN") added for the CLI build.

@rwjuk rwjuk merged commit ab8e895 into develop May 31, 2017
@IntelOrca IntelOrca deleted the refactor/nosdl/phase-1b branch July 12, 2017 14:00
janisozaur added a commit that referenced this pull request Jul 12, 2017
- Feature: [#1399 (partial), #5177] Add window that displays any missing/corrupt objects when loading a park
- Feature: [#5056] Add cheat to own all land.
- Feature: [#5133] Add option to display guest expenditure (as seen in RCTC).
- Feature: [#5196] Add cheat to disable ride ageing.
- Feature: [#5504] Group vehicles into ride groups
- Feature: [#5576] Add a persistent 'display real names of guests' setting.
- Feature: [#5611] Add support for Android
- Feature: [#5706] Add support for OpenBSD
- Feature: OpenRCT2 now starts up on the display it was last shown on.
- Feature: Park entrance fee can now be set to amounts up to £200.
- Improved: Construction rights can now be placed on park entrances.
- Improved: Mouse can now be dragged to select scenery when saving track designs
- Fix: [#259] Money making glitch involving swamps (original bug)
- Fix: [#441] Construction rights over entrance path erased (original bug)
- Fix: [#578] Ride ghosts show up in ride list during construction (original bug)
- Fix: [#597] 'Finish 5 roller coasters' goal not properly checked (original bug)
- Fix: [#667] Incorrect banner limit calculation (original bug)
- Fix: [#739] Crocodile Ride (Log Flume) never allows more than five boats (original bug)
- Fix: [#837] Can't move windows on title screen to where the toolbar would be (original bug)
- Fix: [#1705] Time Twister's Medieval entrance has incorrect scrolling (original bug)
- Fix: [#3178, #5456] Paths with non-ASCII characters not handled properly on macOS.
- Fix: [#3346] Crash when extra long train breaks down at the back
- Fix: [#3479] Building in pause mode creates too many floating numbers, crashing the game
- Fix: [#3565] Multiplayer server crash
- Fix: [#3681] Steel Twister rollercoaster always shows all track designs
- Fix: [#3846, #5749] Crash when testing coaster with a diagonal lift in block brake mode
- Fix: [#4054] Sorting rides by track type: Misleading research messages
- Fix: [#4055] Sort rides by track type: Sorting rule is not really clear (inconsistent?)
- Fix: [#4512] Invisible map edge tiles corrupted
- Fix: [#5009] Ride rating calculations can overflow
- Fix: [#5253] RCT1 park value conversion factor too high
- Fix: [#5400] New Ride window does not focus properly on newly invented ride.
- Fix: [#5489] Sprite index crash for car view on car ride.
- Fix: [#5730] Unable to uncheck 'No money' in the Scenario Editor.
- Fix: [#5750] Game freezes when ride queue linked list is corrupted.
- Fix: [#5819] Vertical multi-dimension coaster tunnels drawn incorrectly
- Fix: Non-invented vehicles can be used via track designs in select-by-track-type mode.
- Fix: Track components added by OpenRCT2 are now usable in older scenarios.
- Technical: [#5047] Add ride ratings tests
- Technical: [#5458] Begin offering headless build with reduced compile- and run-time dependencies
- Technical: [#5755] Title sequence wait periods use milliseconds
- Technical: Fix many desync sources
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.

None yet

9 participants