Skip to content

libretro core #1215

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

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

warmenhoven
Copy link
Contributor

For context I didn't actually write most of this, it was brought over from the citra libretro core. My goal is to support it and improve on it over time.

I've tried to trim down the changes to as small as I thought I could get away with, in the hopes that it could be merged into this repo, and make it easier to maintain long term.

There are some changes, particularly to the CMakeLists.txt files, that are there to satisfy the libretro buildbots. I'm happy to get into the details of exactly what the buildbots are, but the basic point is that they frequently use outdated tools and toolchains in order to support older OS versions or more esoteric hardware. You'll also note that there is the addition of a .gitlab-ci.yml file, for the purpose of integrating with the libretro buildbot. I've currently already got it building there, https://git.libretro.com/warmenhoven/azahar/-/pipelines/369383.

There are still a number of major bugs that need to be addressed before this can reasonably be merged, but I wanted to submit a draft PR early to start getting feedback.

@DavidRGriswold
Copy link
Collaborator

Is it normal for the libretro core for an emulator to live in the emulator repository like this? I feel like im should be its own fork or even its own full repository like https://github.com/JesseTG/melonds-ds is there a reason this approach is better?

@warmenhoven
Copy link
Contributor Author

Is it normal for the libretro core for an emulator to live in the emulator repository like this? I feel like im should be its own fork or even its own full repository like https://github.com/JesseTG/melonds-ds is there a reason this approach is better?

Great question. It's largely up to the upstream emulator devs and the core author. Flycast, ppsspp, Panda3DS, and a number of others live in the emulator repository. However I wouldn't say that has been the norm. The vast majority of them live in a fork in the libretro GitHub org, https://github.com/libretro/. Comparing living directly in upstream vs forked (even soft-forked, like most are) directly into the libretro org, each has advantages and disadvantages. Living directly in upstream means the build is more prone to breaking regularly, as most of the upstream CI is based on GitHub actions and don't build the libretro core; however, the core gets the latest updates immediately, and painful rebases don't happen (which is usually the cause of some notable cores being outdated, including for a long time, citra).

For MelonDS DS, Jesse tried a different approach, where the core itself consumed the melonds emulator as a library; you'll see the cmake files in melonds-ds clone melonds at a specific git hash. He was trying to avoid frequent build breaks, and also avoid the pain of rebases. I am skeptical that he will actually achieve the goal of avoiding integration pain; I think he's just moved the problem from "painful rebase" to "breaking API/functionality changes" that still will require a lot of effort to keep updated. Time will tell.

Personally I prefer having it live in the upstream, which is why I've created this PR here. Of course that depends entirely on the Azahar devs preferences as well. If they're not interested, just close this PR, no hard feelings.

@OpenSauce04
Copy link
Member

Closes #1203

Copy link
Member

@PabloMK7 PabloMK7 left a comment

Choose a reason for hiding this comment

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

I'll continue reviewing later, a lot of changes ^^'

So far I've seen the following.

JNI_PATH: .
CORENAME: azahar
API_LEVEL: 21
BASE_CORE_ARGS: -DENABLE_LIBRETRO=ON -DENABLE_SDL2=OFF -DENABLE_QT=OFF -DENABLE_TESTS=OFF -DENABLE_ROOM=OFF -DENABLE_WEB_SERVICE=OFF -DENABLE_SCRIPTING=OFF -DENABLE_CUBEB=OFF -DENABLE_OPENAL=OFF -DENABLE_LIBUSB=OFF -DCITRA_WARNINGS_AS_ERRORS=OFF
Copy link
Member

Choose a reason for hiding this comment

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

Why disable scripting? It can be controlled by config, and it should be usable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Frontends are already able to implement their own similar scripting functionality (RetroArch has already done this). It shouldn't be necessary to handle it at the core layer.

Copy link
Member

Choose a reason for hiding this comment

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

How do they do it? With Azahar one cannot simple read from the emulated RAM and expect it to be consistent between launches (depends on user configuration). That's why Azahar has the RPC server, which allows select the proper process and accessing the right virtual memory.

if(ANDROID)
cmake_minimum_required(VERSION 3.25)
else()
cmake_minimum_required(VERSION 3.23)
Copy link
Member

Choose a reason for hiding this comment

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

I guess you verified lto works when compiling for Android? What is the build environment when generating an Android 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.

Second question first, this is the Dockerfile that was used to create the android build container: https://git.libretro.com/libretro-infrastructure/libretro-build-android/-/blob/master/Dockerfile?ref_type=heads. You can pull down the docker image from git.libretro.com:5050/libretro-infrastructure/libretro-build-android:latest

I've verified that the core runs :) Yes I do believe that LTO is correctly enabled for Android.

Copy link
Member

Choose a reason for hiding this comment

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

So, when building do you see the LTO enabled message and no error mentioning gold linker unknown? (Or something along those lines)

# Some submodules like to pick their own default build type if not specified.
# Make sure we default to Release build type always, unless the generator has custom types.
if(NOT CMAKE_BUILD_TYPE AND NOT CMAKE_CONFIGURATION_TYPES)
set(CMAKE_BUILD_TYPE "Release" CACHE STRING "Choose the type of build." FORCE)
endif()

if (APPLE)
if (APPLE AND NOT LIBRETRO)
Copy link
Member

Choose a reason for hiding this comment

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

typo? ENABLE_LIBRETRO

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, thank you.

LOG_DEBUG(Frontend, "Initialising core...");

// Set up LLE cores
for (const auto& service_module : Service::service_module_map) {
Copy link
Member

Choose a reason for hiding this comment

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

Why is this forcing all LLE modules off? Am I missing something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will research this and the following comments on citra_libretro.cpp and get back to you. As I mentioned, I just brought over the code from citra and got it running; I haven't gone through and done a thorough audit of it myself yet. That will definitely happen before I take this PR out of draft state.

Copy link
Member

Choose a reason for hiding this comment

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

Now that I think about it, it may just be populating the map in the default state. To be verified.

cpuScale.append("|");
}

retro_variable values[] = {
Copy link
Member

Choose a reason for hiding this comment

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

There seems to be missing options here.


Settings::values.current_input_profile.touch_device = "engine:emu_window";

// Hardcode buttons to bind to libretro - it is entirely redundant to have
Copy link
Member

Choose a reason for hiding this comment

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

Can you explain this?

std::string errorContent = Core::System::GetInstance().GetStatusDetails();
std::string msg;

switch (result) {
Copy link
Member

Choose a reason for hiding this comment

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

Check if we need to handle extra stuff

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants