Get libretro core compiling for Windows #198

Jamiras · 2024-11-29T20:40:26Z

Closes #49

I apologize about the size of the PR, but this is really what it took.

Highlight of changes:

Changed checks on _MSC_VER to _WIN32.
Resources are converted to array data and embedded in the libretro core.
Replace IDirectSoundBuffer psuedo-class with SoundBufferBase base class and SoundBuffer implementation class.
- SoundBufferBase replaces LPDIRECTSOUNDBUFFER in VOICE class. Methods previously acting directly on the LPDIRECTSOUNDBUFFER object now route through the class instance.
- Original DirectSound implementation moved to DSSoundBuffer subclass.
- DSInit and DSUninit are now replaced, and the alternate implementations primarily just define which subclass of SoundBufferBase should be initialized.
- This allows the alternate implementation in an environment that already has DirectX defines (Windows) and constitutes the largest majority of the changes in this PR. Without these changes, the attempted indirections implemented in IDirectSoundBuffer would not line up with the vtable for the actual IDirectSoundBuffer defined in DirectX.
Many printf type statements had to be modified to explicitly cast to the format type (i.e. HRESULT -> unsigned). The compiler was raising warnings/errors that would interrupt the compilation.
Some files were conditionally removed from the common2 library as they are not needed for the libretro build and were causing undesirable dependencies on boost. More information will be provided in comments near the affected components.
Additional notes will be added near relevant pieces of code.

This PR strictly focuses on getting the existing code to compile a Windows core. All four Linux projects still build and seem to be working (on Ubuntu). My testing focus has been the Windows core.

I would like to make further changes to the core behavior in future PRs:

L/R buttons should not modify configuration - let the configuration menu in the frontend handle that.
Double-start should not reset the emulator - let the frontend handle that. This is especially important for achievements, as the achievement logic is reset on emulator reset, and there's no way to know a reset internal to the core has occurred. Also, It's common for users to hit Start to pause their game, and it's too easy to accidentally double tap it.
Double-select should not try to quit. The libretro documentation says "Should only be used if game has a specific way to shutdown the game from a menu item or similar." This is implemented as a hotkey, not in-game functionality.
Need some way to switch disks in drive two. I recommend using the L1/R1 buttons similar to how the PC-8800 core does it. This would also simplify switching disks in drive one.
Alt buttons seem to be bound to action keys (Return, Space, or ESC maybe)?

Jamiras · 2024-11-29T20:41:46Z

CMakeLists.txt

@@ -26,6 +26,10 @@ if (CMAKE_CXX_COMPILER_ID STREQUAL "GNU")
  add_compile_options(-Werror=format -Wno-error=format-overflow -Wno-error=format-truncation -Wno-psabi)
 endif()

+if (BUILD_LIBRETRO)
+  set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -static-libgcc -static-libstdc++")
+endif()


libretro should statically link to as much as possible to prevent possible missing dependency errors and/or conflicting versions of dependencies between the core and the frontend.

Jamiras · 2024-11-29T20:42:22Z

source/CMakeLists.txt

@@ -37,8 +37,6 @@ if ("${SLIRP_FOUND}" STREQUAL "")
  endif()
 endif()

-find_package(Boost REQUIRED)


boost is now only pulled in to the common2 library, and only if building the files dependent on it.

Jamiras · 2024-11-29T20:43:40Z

source/CMakeLists.txt

-  linux/network/portfwds.cpp
+  linux/resources.cpp
+  linux/soundbuffer.cpp
+  linux/version.cpp


This directory should be moved. They're no longer linux-specific. I couldn't think of a good name. Maybe they should be moved into the common2 folder?

Jamiras · 2024-11-29T20:44:33Z

source/CMakeLists.txt

@@ -287,7 +314,8 @@ target_compile_options(appleii PUBLIC

 add_custom_command(
  TARGET appleii POST_BUILD
-  COMMAND ${CMAKE_COMMAND} -E copy ${CMAKE_SOURCE_DIR}/bin/*.SYM ${CMAKE_BINARY_DIR}


msys2 didn't like the *.SYM, so I split it into two statements.

Jamiras · 2024-11-29T20:45:09Z

source/Common.h

@@ -140,7 +140,7 @@ enum AppMode_e
 #define WM_USER_FULLSCREEN	WM_USER+8
 #define VK_SNAPSHOT_TEXT	WM_USER+9 // PrintScreen+Ctrl

-#ifdef _MSC_VER
+#ifdef _WIN32


_MSC_VER is only defined when building with Visual Studio. I've changed most of the _MSC_VER checks to _WIN32.

Jamiras · 2024-11-29T20:50:16Z

source/frontends/libretro/diskcontrol.cpp

@@ -28,13 +28,23 @@ namespace
    return std::equal(prefix.begin(), prefix.end(), value.begin());
  }

+  static std::string fileStem(const std::string& path)


I'm not sure if the msys2 compiler doesn't support c++17, or if the CMake setting isn't being honored, but the compiler couldn't find string::stem or string::native.

compiler or linker error?

Compiler. I might have been misreading it. It's not "not finding string::stem", it's having issues converting value types:

error: no match for 'operator=' (operand types are 'std::string' {aka 'std::__cxx11::basic_string<char>'} and 'std::filesystem::__cxx11::path') 47 | label = path.stem();

and

error: invalid initialization of reference of type 'const std::string&' {aka 'const std::__cxx11::basic_string<char>&'} from expression of type 'const std::filesystem::__cxx11::path::string_type' {aka 'const std::__cxx11::basic_string<wchar_t>'} 108 | myImages.push_back({ filePath.native(), fileStem(filePath.native()), writeProtected, createIfNecessary }); | ~~~~~~~~~~~~~~~^~

using u8string in both cases works, so I've removed the fileStem helper.

Jamiras · 2024-11-29T20:51:59Z

source/frontends/libretro/game.cpp

-namespace
-{
-
-  void saveRegistryToINI(const std::shared_ptr<common2::PTreeRegistry> & registry)


libretro cores should not write files, and i couldn't come up with a good reason to keep this. the frontend will store the modified configuration according to its implementation.

This is a feature I use to debug issues, it saves its configuration to a file I can reload in a non-liretro run. People don't need to use it, nor does it change anything to the libretro execution, it is write-only.

This is my previous attempt: e7795ab

I will review the whole thing again and extend it to all frontends.

Does that mean this is okay, or you want me to put it back?

Jamiras · 2024-11-29T20:52:46Z

source/frontends/libretro/game.cpp

@@ -47,13 +26,11 @@ namespace ra2
    , myAudioSource(AudioSource::UNKNOWN)
    , myKeyboardType(KeyboardType::ASCII)
  {
-    myLoggerContext = std::make_shared<LoggerContext>(true);
+    myLoggerContext = std::make_shared<LoggerContext>(false);


similar to above, the core should not be writing files unnecessarily. this could be modified to write to the frontend log via log_cb, but I felt it was reasonable to just disable it entirely.

right now, this is premature. will be addressed once the compilation is fixed.

Jamiras · 2024-11-29T20:54:05Z

source/frontends/libretro/retroregistry.cpp

@@ -173,6 +173,53 @@ namespace

 namespace ra2
 {
+  class RetroRegistry : public Registry


Avoid boost dependency by storing values in map of maps.

Jamiras · 2024-11-29T20:58:25Z

source/linux/libwindows/dsound.cpp


-#include <cstring>
-
-IDirectSoundBuffer::IDirectSoundBuffer(LPCDSBUFFERDESC lpcDSBufferDesc)


moved to soundbuffer.cpp

audetto · 2024-11-30T16:16:24Z

Hi, impressive work!

If you are thinking about merging this, I need to warn you about some of the constraints of this project.

This projects does not all any change to any file which is already part of the original AppleWin project. Otherwise, merging becomes a nightmare, while now, I get 1 conflict every 6 months.
You might still see a handful of files with differences, simply because I have not unstreamed them yet.

So, where to go from here?

All the trivial changes, (_MSC_VER, int, casting) should go directly to https://github.com/AppleWin/AppleWin (1 or 2 PRs)
The sound abstraction should be a separate PR, still to the AppleWin repository.

Then they will land here and we will make the relevant changes for linux - libretro.

Given the amount of infrastructure, any change of behaviour needs to be a separate PR which will have its own conversation. (otherwise, 1 single PR thread for 5 changes becomes unreadable).

It might sound very painful and a waste of time, but it is exactly what I have done for linux and the results are an extremely successful integration. (look at the fate of https://github.com/linappleii/linapple to see how important this is).

audetto · 2024-11-30T16:45:54Z

source/frontends/libretro/serialisation.cpp

+    DWORD dwRetVal = GetTempPathA(MAX_PATH, lpTempPathBuffer);
+    if (dwRetVal > MAX_PATH || (dwRetVal == 0))
+    {
+        throw std::runtime_error("Cannot create temporary file");


overall indentation is unfortunately non uniform, but each file's is.

It is time to force a decent formatting standards. I will resuscitate an old attempt AppleWin#1202

fixed.

i noticed the inconsistency, and did my best to maintain it on a per-file basis.

Jamiras · 2024-12-01T23:12:22Z

All the trivial changes, (_MSC_VER, int, casting) should go directly to https://github.com/AppleWin/AppleWin (1 or 2 PRs)

Will open that later today

The sound abstraction should be a separate PR, still to the AppleWin repository.

Will open that after the first PR is accepted.

Then they will land here and we will make the relevant changes for linux - libretro.

Will leave this open for that.

Given the amount of infrastructure, any change of behaviour needs to be a separate PR which will have its own conversation.

Absolutely, which is why I left this blurb in the original text: "I would like to make further changes to the core behavior in future PRs"

audetto · 2024-12-03T19:25:50Z

All the trivial changes, (_MSC_VER, int, casting) should go directly to https://github.com/AppleWin/AppleWin (1 or 2 PRs)

Will open that later today

I saw it, good.

The sound abstraction should be a separate PR, still to the AppleWin repository.

Will open that after the first PR is accepted.

Then they will land here and we will make the relevant changes for linux - libretro.

Will leave this open for that.

Yes we will use it as a reference for what is still missing.
There are definitely some good ideas in your work, which I started and never completed.

I will look at these first

resources
boost
clang-format

audetto · 2024-12-03T19:27:05Z

source/StdAfx.h

@@ -78,11 +82,11 @@ typedef UINT64 uint64_t;
 #include <vector>
 #include <cassert>

-#include "windows.h"
+#include "linux/libwindows/windows.h"


don't touch this. my windows.h is supposed to be a drop in replacement of the real one. if there is an ambiguity, it means there is a problem in the makefile.

The ambiguity is that I didn't know it wasn't Microsoft's windows.h until I had spent several more hours trying to detangle defines.

audetto · 2024-12-03T19:28:17Z

source/frontends/common2/CMakeLists.txt

  utils.h
  timer.h
  speed.h
  )

+if (BUILD_APPLEN OR BUILD_QAPPLE OR BUILD_SA2)
+  set(SOURCE_FILES ${SOURCE_FILES}


I will handle this in a different way. The content of the libraries should not depend on what one is building. Better to split the libraries. Leave it with me.

audetto · 2024-12-03T19:30:26Z

source/frontends/libretro/game.cpp

@@ -290,10 +267,6 @@ namespace ra2
      {
        myFrame->Cycle50ScanLines();
      }
-      if (checkButton(RETRO_DEVICE_ID_JOYPAD_L2))
-      {
-        saveRegistryToINI(myRegistry);


same, no change of behaviour until it compiles

audetto · 2024-12-03T19:33:09Z

source/linux/linuxframe.cpp

@@ -1,7 +1,9 @@
 #include "StdAfx.h"
 #include "linux/linuxframe.h"
 #include "linux/context.h"
+#ifndef _WIN32


this should be handled via SLIRP_FOUND

audetto · 2024-12-03T19:34:14Z

source/linux/registryclass.h

@@ -1,6 +1,9 @@
 #pragma once

-#include "wincompat.h"
+#ifndef _WIN32


this should probably include windows.h

audetto · 2024-12-06T18:57:14Z

source/Debugger/Debug.cpp

@@ -6538,7 +6538,9 @@ bool ParseAssemblyListing ( bool bBytesToMemory, bool bAddSymbols )
 		{
 			*p = 0;
 			//	sscanf( sLine, "%s %s %s %s %s %s %s %s", sAddr1, sByte1, sByte2, sByte3, sLineN, sLabel, sAsm, sParam );
-			sscanf( sLine, "%X", &nAddress );
+			unsigned nScanAddress; // helper variable in case sizeof(DWORD) != sizeof(unsigned)
+			sscanf( sLine, "%X", &nScanAddress );


I suspect this will require more care.

I must define DWORD as unsigned int. Can you check what your version of gcc uses?

We will probably have to use a macro to select the correct print format like we already do for size_t and ptrdiff

Based on my comments here, mingw uses unsigned long int for DWORD.

By sscanfing into a secondary variable, we know exactly which format to use ("%X" is appropriate for unsigned - see here). The assignment then converts that to whatever DWORD is defined as.

Jamiras added 12 commits November 28, 2024 11:20

update CMakeLists for Windows

34b93ed

fix linux build

98addda

replace IDirectSound overrides with wrapper class

9e9fc73

eliminate file access for settings

ac1b249

static link slirp

e077dca

static link gcc/stdc for libretro build

a0fa9f9

don't write AppleWin.log from libretro core

0d2d56e

tabs->spaces

154e5c9

update README

6889044

fix linux builds

45a48e0

use _WIN32 define

af635cd

fix linux build

262ca8a

Jamiras commented Nov 29, 2024

View reviewed changes

revert changes to libwindows; it's not needed by libretro build

72258a5

audetto reviewed Nov 30, 2024

View reviewed changes

audetto added the libretro label Nov 30, 2024

Jamiras added 2 commits December 1, 2024 12:15

move winsock to stdafx, eliminate WIN32_LEAN_AND_MEAN

54c8d4f

address feedback

b248204

Jamiras mentioned this pull request Dec 2, 2024

check _WIN32 instead of _MSC_VER AppleWin/AppleWin#1346

Open

audetto reviewed Dec 3, 2024

View reviewed changes

audetto reviewed Dec 6, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Get libretro core compiling for Windows #198

Get libretro core compiling for Windows #198

Jamiras commented Nov 29, 2024

Jamiras Nov 29, 2024

Jamiras Nov 29, 2024

Jamiras Nov 29, 2024

Jamiras Nov 29, 2024

Jamiras Nov 29, 2024

Jamiras Nov 29, 2024

audetto Nov 30, 2024

Jamiras Dec 1, 2024

Jamiras Nov 29, 2024

audetto Nov 30, 2024 •

edited

Loading

audetto Nov 30, 2024

Jamiras Dec 1, 2024

Jamiras Nov 29, 2024

audetto Dec 3, 2024

Jamiras Nov 29, 2024

Jamiras Nov 29, 2024

audetto commented Nov 30, 2024 •

edited

Loading

audetto Nov 30, 2024

audetto Nov 30, 2024

Jamiras Dec 1, 2024

Jamiras commented Dec 1, 2024

audetto commented Dec 3, 2024

audetto Dec 3, 2024

Jamiras Dec 3, 2024

audetto Dec 3, 2024

audetto Dec 3, 2024

audetto Dec 3, 2024

audetto Dec 3, 2024

audetto Dec 6, 2024

Jamiras Dec 6, 2024


		#include <cstring>

		IDirectSoundBuffer::IDirectSoundBuffer(LPCDSBUFFERDESC lpcDSBufferDesc)

Get libretro core compiling for Windows #198

Are you sure you want to change the base?

Get libretro core compiling for Windows #198

Conversation

Jamiras commented Nov 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

audetto Nov 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

audetto commented Nov 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jamiras commented Dec 1, 2024

audetto commented Dec 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

audetto Nov 30, 2024 •

edited

Loading

audetto commented Nov 30, 2024 •

edited

Loading