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

Cross-platform support? #16

Open
neuromancer opened this issue Jul 28, 2019 · 26 comments

Comments

@neuromancer
Copy link

@neuromancer neuromancer commented Jul 28, 2019

I'm wondering if cross-platform support is planed for the future, in particular if this should run in Linux.

Thanks!

@Try

This comment has been minimized.

Copy link
Owner

@Try Try commented Jul 28, 2019

Hi!
No such plans, until stable windows version is finished. In the meantime pull-requests is welcome :)

In short required changes for Linux support is:

  • assets.cpp, wfile.cpp, rfile.cpp - file I/O support
  • systemapi.cpp - OS main-loop layer
  • vulkanapi_impl.cpp - have to update WSI/X11 support for Vulkan
  • installdetect.cpp - can be disabled, to start with
@dreamer

This comment has been minimized.

Copy link

@dreamer dreamer commented Jul 29, 2019

@Try I haven't tried to compile your project yet, but have you considered using SDL2 instead of coding main loop and windowing subsystem yourself?

@Try

This comment has been minimized.

Copy link
Owner

@Try Try commented Jul 29, 2019

Well I tried SDL in other project, but it was 5 yeas ago, and SDL1 :)

In short, I see very little reason to use a library for it, since Win32 support layer is only 464 lines long. Linux/X11, also should be pretty straight forward to do.

@herzenschein

This comment has been minimized.

Copy link

@herzenschein herzenschein commented Sep 15, 2019

Hello, I tried compiling the game on Linux and everything (bullet3, edd-dbg, MoltenTempest) compiled, except daedalus/DaedalusVM.cpp, as follows:

[ 16%] Building CXX object lib/ZenLib/daedalus/CMakeFiles/daedalus.dir/DaedalusVM.o
/home/blumen/OpenGothic/OpenGothic-master/lib/ZenLib/daedalus/DaedalusVM.cpp: In member function ‘void Daedalus::DaedalusVM::eval(uint32_t)’:
/home/blumen/OpenGothic/OpenGothic-master/lib/ZenLib/daedalus/DaedalusVM.cpp:45:50: error: cannot bind non-const lvalue reference of type ‘size_t& {aka long unsigned int&}’ to an rvalue of type ‘size_t {aka long unsigned int}’
const PARStackOpCode& op = nextInstruction(PC);
^ (this points to the) in (PC))
In file included from /home/blumen/OpenGothic/OpenGothic-master/lib/ZenLib/daedalus/DaedalusVM.cpp:5:0:
/home/blumen/OpenGothic/OpenGothic-master/lib/ZenLib/daedalus/DaedalusVM.h:69:27: note: initializing argument 1 of ‘const Daedalus::PARStackOpCode& Daedalus::DaedalusVM::nextInstruction(size_t&)’
const PARStackOpCode &nextInstruction(size_t& pc);
^~~~~~~~~~~~ (this points to nextinstruction)
lib/ZenLib/daedalus/CMakeFiles/daedalus.dir/build.make:158: recipe for target 'lib/ZenLib/daedalus/CMakeFiles/daedalus.dir/DaedalusVM.o' failed
make[2]: *** [lib/ZenLib/daedalus/CMakeFiles/daedalus.dir/DaedalusVM.o] Error 1
CMakeFiles/Makefile2:569: recipe for target 'lib/ZenLib/daedalus/CMakeFiles/daedalus.dir/all' failed
make[1]: *** [lib/ZenLib/daedalus/CMakeFiles/daedalus.dir/all] Error 2
Makefile:129: recipe for target 'all' failed
make: *** [all] Error 2

In addition, there's a pull request with this commit which fixes DaedalusExcept.cpp. I'm not very acquainted with Github so I don't know if it's better to report this upstream.

@Try

This comment has been minimized.

Copy link
Owner

@Try Try commented Sep 17, 2019

@herzenschein
I assume you building it for x64 Linux, in this case:
PC is uint32_t variable, but nextInstruction expects a size_t&, aka uint64_t&.

So possible resolution is to change PC from uint32_t to size_t.

@herzenschein

This comment has been minimized.

Copy link

@herzenschein herzenschein commented Sep 17, 2019

Ok! So I changed void DaedalusVM:eval(uint32_t PC) to void DaedalusVM:eval(size_t PC) in DaedalusVM.cpp.
I also needed to change void eval(uint32_t pcInit) to void eval(size_t pcInit) in DaedalusVM.h.
libvulkan-dev is also a requirement because of vulkan.hpp.

The next error for compilation was:
/home/blumen/OpenGothic/lib/MoltenTempest/Engine/graphics/textureatlas.cpp: In member function ‘void Tempest::TextureAtlas::emplace(Tempest::TextureAtlas::Allocation&, const void*, uint32_t, uint32_t, Tempest::Pixmap::Format, uint32_t, uint32_t)’:
/home/blumen/OpenGothic/lib/MoltenTempest/Engine/graphics/textureatlas.cpp:56:9: error: ‘memcpy’ was not declared in this scope
memcpy(data+((y+iy)dw+dx),src+iysw,sw);
^~~~~~
/home/blumen/OpenGothic/lib/MoltenTempest/Engine/graphics/textureatlas.cpp:56:9: note: suggested alternative: ‘wmemcpy’
memcpy(data+((y+iy)dw+dx),src+iysw,sw);
^~~~~~
wmemcpy
make[2]: *** [lib/MoltenTempest/Engine/CMakeFiles/MoltenTempest.dir/graphics/textureatlas.cpp.o] Error 1
make[1]: *** [lib/MoltenTempest/Engine/CMakeFiles/MoltenTempest.dir/all] Error 2
make: *** [all] Error 2

If I try changing memcpy(data+ to wmemcpy(data+ in MoltenTempest/Engine/graphics/textureatlas.cpp:56:9, as suggested by make, the following error appears:
/home/blumen/OpenGothic/lib/MoltenTempest/Engine/graphics/textureatlas.cpp: In member function ‘void Tempest::TextureAtlas::emplace(Tempest::TextureAtlas::Allocation&, const void*, uint32_t, uint32_t, Tempest::Pixmap::Format, uint32_t, uint32_t)’:
/home/blumen/OpenGothic/lib/MoltenTempest/Engine/graphics/textureatlas.cpp:56:49: error: cannot convert ‘unsigned char*’ to ‘wchar_t*’ for argument ‘1’ to ‘wchar_t* wmemcpy(wchar_t*, const wchar_t*, size_t)’
wmemcpy(data+((y+iy)dw+dx),src+iysw,sw);
^ (it points to the last ')' )
make[2]: *** [lib/MoltenTempest/Engine/CMakeFiles/MoltenTempest.dir/graphics/textureatlas.cpp.o] Error 1
make[1]: *** [lib/MoltenTempest/Engine/CMakeFiles/MoltenTempest.dir/all] Error 2
make: *** [all] Error 2

I should mention that I'm not a programmer, just an intermediate linux user. I'm not sure if this is the correct place to mention these compilation steps.

@Try

This comment has been minimized.

Copy link
Owner

@Try Try commented Sep 17, 2019

error: ‘memcpy’ was not declared in this scope

#include <cstring>

I'm not sure if this is the correct place to mention these compilation steps.

Well... I don't think posting all compilation errors in ticket is going to work - expect to have 50-100 of them in total. Apart from compile errors, there is also #16 (comment) to be done.

@Try

This comment has been minimized.

Copy link
Owner

@Try Try commented Oct 9, 2019

Linux/Ubuntu build is now available: https://ci.appveyor.com/project/Try/opengothic/build/job/xxrkosnfwmh20p6x
At this point 'make it compile' stage is finished, still to be done:

  • adjust Dir::scan for linux
  • implement event-switch for X11
  • run and test the result

FYI: VirtualBox doesn't support Vulkan, so don't expect to see full Linux support in next release.

@swick

This comment has been minimized.

Copy link
Contributor

@swick swick commented Oct 9, 2019

implement event-switch for X11

If you value your sanity: use SDL2.

@Try

This comment has been minimized.

@swick

This comment has been minimized.

Copy link
Contributor

@swick swick commented Oct 10, 2019

Great, now test it on 20 different window managers and reimplement it all for wayland and dynamically choose the right backend… or don't and just add SDL2.

@ChristophHaag

This comment has been minimized.

Copy link

@ChristophHaag ChristophHaag commented Nov 8, 2019

What's the status here? The ubuntu build on appveyor does not build a lot of the stuff, for example the vulkan renderer.

I tried to compile OpenGothic, but there are many things that are still windows only, for example in lib/MoltenTempest/Engine/io/rfile.cpp and lib/MoltenTempest/Engine/gapi/vulkan/vkdevice.cpp.

Are you planning on porting all of it to Linux?

@Try

This comment has been minimized.

Copy link
Owner

@Try Try commented Nov 11, 2019

Hi, @ChristophHaag !

Linux build issues can bump-up from time-to-time, but in general it's build-able.
Now, it's green: https://ci.appveyor.com/project/Try/opengothic/build/job/hwcw9xyi6buqg9lg
Porting of rfile.cpp/vulkan-stuff is written; window event loop - not implemented.
Of course, nothing linux-related is tested yet.

At this moment Linux is not a top priority for me - priority is the game itself.
On top of it I don't have any linux machine, only virtual-box with no vulkan support :(

@ChristophHaag

This comment has been minimized.

Copy link

@ChristophHaag ChristophHaag commented Nov 11, 2019

Sorry, my mistake. My submodules were not up to date.

It does build. It does not work though I think it's at a point where someone else (perhaps me) can look into fixing it up.

Thread 1 "Gothic2Notr" received signal SIGABRT, Aborted.
0x00007ffff7805f25 in raise () from /usr/lib/libc.so.6
(gdb) bt
#0  0x00007ffff7805f25 in raise () from /usr/lib/libc.so.6
#1  0x00007ffff77ef897 in abort () from /usr/lib/libc.so.6
#2  0x0000555555b684aa in terminateHandler () at ../Game/utils/crashlog.cpp:71
#3  0x00007ffff7b964da in __cxxabiv1::__terminate (handler=<optimized out>) at /build/gcc/src/gcc/libstdc++-v3/libsupc++/eh_terminate.cc:47
#4  0x00007ffff7b96537 in std::terminate () at /build/gcc/src/gcc/libstdc++-v3/libsupc++/eh_terminate.cc:57
#5  0x00007ffff7b9678e in __cxxabiv1::__cxa_throw (obj=<optimized out>, tinfo=0x7ffff7cd0750 <typeinfo for std::system_error>, dest=0x7ffff7bc2c60 <std::system_error::~system_error()>) at /build/gcc/src/gcc/libstdc++-v3/libsupc++/eh_throw.cc:95
#6  0x0000555555d6a644 in Tempest::Detail::VDevice::VDevice (this=0x555556776ef0, api=..., hwnd=0x555556137cb0) at ../lib/MoltenTempest/Engine/gapi/vulkan/vdevice.cpp:100
#7  0x0000555555d04a17 in Tempest::VulkanApi::createDevice (this=0x7fffffffbf70, w=0x555556137cb0) at ../lib/MoltenTempest/Engine/gapi/vulkanapi.cpp:42
#8  0x0000555555d0bf98 in Tempest::Device::Impl::Impl (this=0x7fffffffc278, api=..., w=0x555556137cb0, maxFramesInFlight=2 '\002') at ../lib/MoltenTempest/Engine/graphics/device.cpp:21
#9  0x0000555555d0c114 in Tempest::Device::Device (this=0x7fffffffc268, api=..., w=0x555556137cb0, maxFramesInFlight=2 '\002') at ../lib/MoltenTempest/Engine/graphics/device.cpp:38
#10 0x0000555555b1382c in MainWindow::MainWindow (this=0x7fffffffc190, gothic=..., api=...) at ../Game/mainwindow.cpp:28
#11 0x0000555555b124f7 in main (argc=3, argv=0x7fffffffdb18) at ../Game/main.cpp:18
@swick

This comment has been minimized.

Copy link
Contributor

@swick swick commented Nov 12, 2019

The windowing and swapchain stuff is broken. Would be easy to fix by using SDL2 but a certain someone has very strong opinions and believes that he can do everything better on his own. I've given up on this project.

@Try

This comment has been minimized.

Copy link
Owner

@Try Try commented Nov 12, 2019

@ChristophHaag you have exception at line 100 of vdevice.cpp
It means, that engine wasn’t able to find any suitable physical device.
The can be 2 common reasons for it:

  • you have Vulkan, but only headless(without graphics queue)
  • I’m asking too much in isDeviceSuitable
@swick

This comment has been minimized.

Copy link
Contributor

@swick swick commented Nov 12, 2019

Honestly, I don't get how your code works on Windows because it's obviously broken. You're missing the platform specific instance surface extensions. I said this before: you don't know this stuff better than SDL2. Case in point. This kind of stuff is exactly why I don't give a fuck about this project anymore.

@ChristophHaag

This comment has been minimized.

Copy link

@ChristophHaag ChristophHaag commented Nov 12, 2019

The problem is vkGetPhysicalDeviceSurfaceSupportKHR(device,i,surface,&presentSupport);. As far as I can see from the vulkan caps viewer database, none of the drivers on linux support presentation on any queue family.

I don't know a lot of Vulkan, but my guess is that you will need to use a platform specific function like vkGetPhysicalDeviceXlibPresentationSupportKHR to query support for presenting to an X11 window etc.

While I don't agree with the very dismissive tone of some other comments here (you can develop your project however you like), SDL would probably make platform independent windowing system integration a lot easier.

@swick

This comment has been minimized.

Copy link
Contributor

@swick swick commented Nov 12, 2019

I already told you exactly what's wrong with the code. No queue has present capabilities because there literally is no WSI loaded anywhere. The code should not even work on windows.

While I don't agree with the very dismissive tone of some other comments here

Yeah, well, if you fix all the stupid mistakes someone made that could have been avoided by using standard libraries and then get told that he won't use the libraries because he knows everything better (which he obviously doesn't, otherwise the code would not be so trivially broken) you can get in a bad mood.

@Try

This comment has been minimized.

Copy link
Owner

@Try Try commented Nov 13, 2019

@ChristophHaag you make a good point about vkGetPhysicalDeviceXlibPresentationSupportKHR. I’m going to take a closer look on it, on a weekend(right now, I’m on a travel without access to computer).
But, yes - we need to do more adjustments for Vulkan on Linux.

@ChristophHaag

This comment has been minimized.

Copy link

@ChristophHaag ChristophHaag commented Nov 13, 2019

It was pointed out to me that vulkan caps viewer lists present support as always false because it hasn't actually implemented that query for linux: https://github.com/SaschaWillems/VulkanCapsViewer/blob/3db554165af9b860dca98a216742d2d7df9a1bf0/vulkanDeviceInfo.cpp#L148. But the commented out code does use the xcb platform specific query.

The other part of my comment is still true, I had checked that vkGetPhysicalDeviceSurfaceSupportKHR() in the OpenGothic code does set presentSupport to false for all queue families.

@ChristophHaag

This comment has been minimized.

Copy link

@ChristophHaag ChristophHaag commented Nov 13, 2019

Was a comment from swick deleted? No, adding VK_KHR_XLIB_SURFACE_EXTENSION_NAME to the instance extension still doesn't make vkGetPhysicalDeviceSurfaceSupportKHR report present support.

By the way: the validation layer has been renamed in recent vulkan, it's now VK_LAYER_KHRONOS_validation. The old ones may still exist for compatibility reasons I guess, but may not do anything.

@swick

This comment has been minimized.

Copy link
Contributor

@swick swick commented Nov 13, 2019

diff --git a/Engine/gapi/vulkan/vdevice.cpp b/Engine/gapi/vulkan/vdevice.cpp
index 6b62bd2..35b684d 100644
--- a/Engine/gapi/vulkan/vdevice.cpp
+++ b/Engine/gapi/vulkan/vdevice.cpp
@@ -139,7 +139,7 @@ void VDevice::createSurface(VulkanApi &api,void* hwnd) {
   VkXlibSurfaceCreateInfoKHR createInfo = {};
   createInfo.sType  = VK_STRUCTURE_TYPE_XLIB_SURFACE_CREATE_INFO_KHR;
   createInfo.dpy    = reinterpret_cast<Display*>(X11Api::display());
-  createInfo.window = Window(hwnd);
+  createInfo.window = *(reinterpret_cast<Window*>(hwnd));
   if(vkCreateXlibSurfaceKHR(api.instance, &createInfo, nullptr, &surface)!=VK_SUCCESS)
     throw std::system_error(Tempest::GraphicsErrc::NoDevice);
 #else
diff --git a/Engine/gapi/vulkan/vulkanapi_impl.cpp b/Engine/gapi/vulkan/vulkanapi_impl.cpp
index d9109df..0bc6f3b 100644
--- a/Engine/gapi/vulkan/vulkanapi_impl.cpp
+++ b/Engine/gapi/vulkan/vulkanapi_impl.cpp
@@ -50,6 +50,7 @@ VulkanApi::VulkanApi(bool validation)
     VK_EXT_DEBUG_REPORT_EXTENSION_NAME,
     VK_KHR_SURFACE_EXTENSION_NAME,
     SURFACE_EXTENSION_NAME,
+    VK_KHR_XLIB_SURFACE_EXTENSION_NAME,
     };
 
   createInfo.enabledExtensionCount   = static_cast<uint32_t>(extensions.size());

Have fun. It'll crash somewhere in the font loader. The quality of the code is just not great and the hubris of the author makes this a deadly combination.

@swick

This comment has been minimized.

Copy link
Contributor

@swick swick commented Nov 13, 2019

diff --git a/Engine/io/rfile.cpp b/Engine/io/rfile.cpp
index 4a88cba..8ae5609 100644
--- a/Engine/io/rfile.cpp
+++ b/Engine/io/rfile.cpp
@@ -87,7 +87,7 @@ size_t RFile::read(void *dest, size_t size) {
 
   return cnt;
 #else
-  return fread(dest,size,1,reinterpret_cast<FILE*>(handle));
+  return fread(dest,1,size,reinterpret_cast<FILE*>(handle));
 #endif
   }

This gets you to the mainloop. Have fun messing with X11 and xlib then because the mainloop for X11 is broken and I certainly won't do that.

@Try

This comment has been minimized.

Copy link
Owner

@Try Try commented Nov 14, 2019

@ChristophHaag

Was a comment from swick deleted?

Don't know - wasn't here for almost a day.

By the way: the validation layer has been renamed in recent vulkan, it's now VK_LAYER_KHRONOS_validation

Thanks! Old layer is still working, but update required.

@swick

-  createInfo.window = Window(hwnd);
+  createInfo.window = *(reinterpret_cast<Window*>(hwnd));

This change is correct, but a mentioned in #16 (comment) - linux code is not tested.

     SURFACE_EXTENSION_NAME,
+    VK_KHR_XLIB_SURFACE_EXTENSION_NAME,
     };

This change is wrong: SURFACE_EXTENSION_NAME expands as VK_KHR_XLIB_SURFACE_EXTENSION_NAME on linux, so your change not changing anything for linux, but breaks windows.

 #else
-  return fread(dest,size,1,reinterpret_cast<FILE*>(handle));
+  return fread(dest,1,size,reinterpret_cast<FILE*>(handle));
 #endif

That change is fine.

The quality of the code is just not great and the hubris of the author makes this a deadly combination.

So you found 2 mistakes in ~2k line commit, for linux support and have one in your ~50 line change...

Try added a commit to Try/MoltenTempest that referenced this issue Nov 16, 2019
Try added a commit to Try/MoltenTempest that referenced this issue Nov 17, 2019
Try added a commit to Try/MoltenTempest that referenced this issue Nov 17, 2019
@Try

This comment has been minimized.

Copy link
Owner

@Try Try commented Nov 17, 2019

OK, now I'm officially back.
Spend a lot of time to setup Vulkan on VirtualBox, - still not working.

  • VK_LAYER_KHRONOS_validation - tested on windows, committed
  • vkGetPhysicalDeviceXlibPresentationSupportKHR - can't test, won't commit.
  • Introduce a appveyor build for engine.
  • Add test suite in CI
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.