afritz1 / OpenTESArena

Open-source re-implementation of The Elder Scrolls: Arena.
MIT License
988 stars 68 forks source link

Made project buildable with MSYS2 + MinGW #60

Closed ghost closed 7 years ago

ghost commented 7 years ago

The original cl2.hpp from KhronosGroup file causes 'multiple definition' errors when using MinGW. This was caused due to the 'default' objects from cl::Device, Platform, Context and CommandQueue being defined in every file that #included cl2.hpp. I have moved the definitions to cl2.cpp so that they are defined in only one place.

I've also changed opentesarena.rc's encoding to ANSI so that it compiles with MinGW. It should still compile fine with other compilers.

afritz1 commented 7 years ago

Thanks for looking into this.

In the .gitignore, the data and options folders actually shouldn't be ignored. What I intended is for developers to have a build folder that they add those data and options folders to as local copies for their executable, and the folders in the root directory are where the "official" changes go. That's how I'm using it with Visual Studio.

I'm not sure I want to have OpenCL source files in the repository except as a last resort. The conditional include in CLProgram.h looks kind of like a hack, too. Maybe there should be some changes to cmake/FindOpenCL.cmake instead? That .cmake file actually isn't 100% correct at the moment, since it finds the 32-bit OpenCL library even when compiling on 64-bit mode (the CMAKE_SIZEOF_VOID_P code in there was stripped out for some reason; it should probably go back in).

In any case, I don't think cl2.hpp is perfect. When I tried compiling a 32-bit executable in Visual Studio, the compiler complained about some parameters in cl2.hpp (cl_float4 I think), and I had to change them from copies to const& on my machine. That's not really an ideal solution, so having a customized cl2.hpp in the repository might end up being a necessary evil. Alternatively, I could see if switching to cl.hpp would be an option, or even cl.h as the extreme.

ghost commented 7 years ago

Alright, thanks for explaining the situation with the data and options folders.

KhronosGroup won't fix this issue (KhronosGroup/OpenCL-CLHPP#17), and adding a .cpp file to their repository would defeat the purpose of having a simple to use .hpp library.

We could try using cl.hpp, but if that doesn't work, I could try making CMake download and modify the file on the fly if MinGW is detected.

afritz1 commented 7 years ago

You're able to get those changes to compile with MinGW? I'm trying them out in Visual Studio but it seems there also needs to be #define NOMINMAX before #include <CL/cl.hpp>, because <windows.h> is included by cl.hpp and it defines macros min and max, causing problems for std::min and std::max.

ghost commented 7 years ago

Yes, the AMD SDK's cl.hpp defines its own min and max macros. You can't really do much about it. I have the NVIDIA SDK, but I don't use their headers. Instead I built one using KhronosGroup/OpenCL-CLHPP. You could try this, but it requires you to have Python installed and it might not make a difference.

ghost commented 7 years ago

I'm going to check back on this project in about 19 hours. Since using cl.hpp might be inconvenient to some users, I'll try to revert back to cl2.hpp and modify CMakeLists.txt to make a special case for MinGW. Please don't spend too much time worrying about this issue - I don't want to cause unnecessary trouble.

afritz1 commented 7 years ago

No worries! To tell the truth, the more I've been using OpenCL, the more I've learned that it's not the best choice for graphics in the long run. First off, it's not a graphics API, so the rendering performance will never be above "average" (add this to the software rendering requirement for SDL2 on Linux), unless the pipeline is combined with something else like OpenGL. Additionally, Nvidia only supports up to version 1.2 (circa Nov. 2011), and that's a little inconvenient to work around.

Anyway, when I started this project, OpenCL sounded like a fun idea and it's been decent for a while, but now it's starting to become more trouble than it's worth, and the performance isn't really where I'd like it to be. Once I've learned more about Vulkan, I'll be happy to start moving the rendering code over to that instead, though it will probably take some time.