Full GBA Controllers Integration with mGBA #9600
Conversation
|
So Sonic Adventure not connecting is because the most common guide appears to be incorrect. I need to get around to doing what the booklet says to do lol. Edit: Nope, still doesn't work. Guides were correct enough anyway and the booklets are even more vague than the guide. This is a case where it should just work, but doesn't. As endrift pointed out below, there's probably more bugs in the protocol. |
|
@endrift this might interest you as well |
|
could this be used to get Gameboy Player to work in the future? |
There was a problem hiding this comment.
This seems exceedingly fragile and inclined to break at any given moment when I update, well, pretty much anything upstream, especially since some of the APIs used are going to be changing in the future. While having it as an option seems neat, I'm extremely unfond of wholesale replacing the old SI_DeviceGBA since it means that your version of Dolphin and your version of mGBA are tightly integrated. This has a huge number of issues, including causing a large burden on bug reporting for games that have been fixed upstream but not in your downstream, making packaging difficult for distros, and a lot more. Furthermore, as evidenced I've done a bunch of work lately on fixing the old version to work better (with still more room to go) so this kind of feels like a slap in the face.
Source/Core/Core/HW/GBACore.cpp
Outdated
| static VFile* LoadROM_Zip(const char* path) | ||
| { | ||
| VFile* vf{}; | ||
| unzFile zip = unzOpen(path); | ||
| if (zip) | ||
| { | ||
| do | ||
| { | ||
| unz_file_info info{}; | ||
| if (unzGetCurrentFileInfo(zip, &info, nullptr, 0, nullptr, 0, nullptr, 0) != UNZ_OK || | ||
| !info.uncompressed_size) | ||
| continue; | ||
|
|
||
| std::vector<u8> buffer(info.uncompressed_size); | ||
| if (!Common::ReadFileFromZip(zip, &buffer)) | ||
| continue; | ||
|
|
||
| vf = VFileMemChunk(buffer.data(), info.uncompressed_size); | ||
| if (mCoreIsCompatible(vf) == mPLATFORM_GBA) | ||
| { | ||
| vf->seek(vf, 0, SEEK_SET); | ||
| break; | ||
| } | ||
|
|
||
| vf->close(vf); | ||
| vf = nullptr; | ||
| } while (unzGoToNextFile(zip) == UNZ_OK); | ||
| unzClose(zip); | ||
| } | ||
| return vf; | ||
| } |
There was a problem hiding this comment.
mGBA has minizip integration built in already. Why did you add a second version?
There was a problem hiding this comment.
Building with LIBMGBA_ONLY will disable every dependency, except for lzma for some reason which is convenient in our case since Dolphin doesn't have 7z support
If anything I'd rather go the opposite direction and disable lzma in mGBA and have Dolphin handle it instead
There was a problem hiding this comment.
If Dolphin is already linking against minizip it would be preferable to use mGBA's existing minizip code and just point it to the right headers.
There was a problem hiding this comment.
Dolphin's minizip doesn't have the zipping functions required for mGBA's vfs-zip. We could update it but that would only introduce dead code
Source/Core/Core/HW/GBACore.cpp
Outdated
| static_cast<s32>((gc_ticks - m_last_gc_ticks) * GBA_ARM7TDMI_FREQUENCY / gc_frequency)); | ||
| m_waiting_for_event = true; | ||
|
|
||
| s32 start_time = mTimingCurrentTime(m_core->timing); |
There was a problem hiding this comment.
You may want to use mTimingGlobalTime which is a u64.
There was a problem hiding this comment.
Turns out that is only available when USE_DEBUGGERS is defined, this is not an issue since we frequently sync devices and already use an s32 for the event anyway
Short answer: not really, but it's complicated |
|
I agree that we should keep the old way of connecting to a GBA emulator as an option. |
|
Especially since connecting via libmgba is enabled or disabled at compile time, removing the old approach seems like a bad idea. |
There was a problem hiding this comment.
I'm pleasently surprised how little code this needed. Also, welcome to the project Bonta (first you conquered ALTTPR, now you conquer emulation? Nice.)
I do wonder if we should keep around the other GBA implementation for...something. Mainly because it had the loose coupling of a GBA implementation (vs. locking us into a specific version of mGBA.)
If we do, I feel like we should make this the user-facing default tho, since it has the It-Just-Works™ factor many end-users seek.
|
I do think we keep around the old path if possible, but at the same time there is a level of usability, especially on Windows, that is lacking with the old protocol. Linux definitely performs better than Windows with the old setup with separate emulators (verified in Crystal Chronicles) but with this everyone is brought to an even playing field and it even works on netplay between different OSes in my experience! There are definitely some protocol bugs, as endrift has already mentioned. We have a lot of unknowns with the protocol as well. Also there's the fact that by removing the old support, we'd be essentially removing support for connecting to VBA-M from Dolphin immediately. They'd still be able to connect to old builds and releases, so it's not a huge problem. And you could talk me into that from a user standpoint considering how big of a usability upgrade this actually is; this makes it super easy to setup and adds a ton of features that users really want. I don't see this as an end-point to GBA <-> GCN, however. I see it as a transitional point with the real tests ahead. This makes things more consistent, more deterministic, and more useable which should result in us being able to track down the remaining bugs more easily without having to worry about latency being unrealistically high like with the old connectivity on Windows. |
|
I've restored the previous GBA implementation, a force-push was necessary to keep a coherent commit history, sorry about that |
|
Don't worry about the force push, we do that all the time in pull requests to keep the history tidy. |
|
I would split the "Movies" and "Netplay" to a different pr(s), to reduce the size of the diff. This would also narrow down what needs to be tested, reducing the possibility of unnoticed bugs from being merged. |
|
Considering it's adding a new feature and things are fairly well segmented between the different modules, I'm not certain that's necessary? The way it is now makes it so everything can be tested in one place and it is pretty explicit about what it adds. |
|
On the latest version of this Pull Request downloaded from buildbot, I can't seem to run any games regardless of controller settings. Hard crash. I will run from Visual Studio to see if I get the same crash and get a stack trace. |
|
Here's the stack trace along with Visual Studio saying what happened.
|
|
@dolphin-emu-bot rebuild |
|
After retesting this and testing both the internal GBA and GBA over TCP, I'm pleased with the performance of this build. I'm able to do things like use savestates, record movies, and even play netplay. And if the old way to do things is always there if I want to connect over TCP, for say playing across the network on a separate device. I know there's a lot of code review to happen and things to discuss on that front, but I'm extremely happy with how well it works. Usage wise, huge thumbs up. |
|
I have included the commit from #9608 for now so this can be tested DSP HLE as well |
|
@dolphin-emu-bot rebuild |
|
Reported issues with Pokemon Box were not related to GBA <-> GCN and were fixed here -> #9625 This should mean that once this is merged along with the timing fixes, all GBA <-> GCN games will be fully supported except one. The true final boss of the GameCube and the last game not to reach gameplay: The GameBoy Player. |
| m_core->init(m_core); | ||
|
|
||
| mCoreInitConfig(m_core, "dolphin"); | ||
| mCoreConfigSetValue(&m_core->config, "idleOptimization", "detect"); |
There was a problem hiding this comment.
I don't think this config setting this is necessary, and I'm not sure if it contributes anything. It probably doesn't hurt anything either though.
There was a problem hiding this comment.
This can be more efficient than the default "remove" in some games. I left it there as a way to potentially avoid stalling Dolphin or lower CPU usage
| set(USE_MGBA 0) | ||
| endif() | ||
| if(USE_MGBA) | ||
| message(STATUS "Using static libmgba from Externals") |
There was a problem hiding this comment.
It's worth noting that some distros do ship libmgba, though they don't tend to ship libmgba-dev.
There was a problem hiding this comment.
That could be a issue for NetPlay or Movies, we may have to enforce a specific version for those cases
There was a problem hiding this comment.
That's a good point. Distros can get angry about it though.
| endif() | ||
|
|
||
| if(ANDROID) | ||
| target_compile_definitions(mgba PRIVATE -Dfutimes=futimens) |
There was a problem hiding this comment.
I should fix this upstream.
There was a problem hiding this comment.
This should be fixed upstream now.
| constexpr auto SAMPLES = 512; | ||
| constexpr auto SAMPLE_RATE = 48000; | ||
|
|
||
| // libmGBA does not return the correct frequency for some GB models |
There was a problem hiding this comment.
I would like to know this too
| #ifdef HAS_LIBMGBA | ||
| {SerialInterface::SIDEVICE_GC_GBA_EMULATED, "GBA (Emulated)"}, | ||
| #endif | ||
| {SerialInterface::SIDEVICE_GC_GBA, "GBA (TCP)"}, |
There was a problem hiding this comment.
I agree that this wording is confusing. Both are emulated after all, just one internal to Dolphin and one external. Perhaps "Integrated" and "Remote".
There was a problem hiding this comment.
Reviewed 15 of 15 files at r1, 4 of 4 files at r2, 2 of 2 files at r3, 7 of 7 files at r4, 2 of 2 files at r5, 3 of 3 files at r6, 2 of 2 files at r7, 15 of 15 files at r8, 10 of 10 files at r9, 3 of 3 files at r10, 6 of 6 files at r11, 4 of 4 files at r12, 9 of 9 files at r13, 7 of 7 files at r14, 8 of 8 files at r15, 9 of 9 files at r16, 16 of 16 files at r17, 5 of 5 files at r18.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @Bonta0)
Source/Core/DolphinQt/GBAHost.cpp, line 28 at r15 (raw file):
u32 width, height; core_ptr->GetVideoDimensions(&width, &height); QMetaObject::invokeMethod(
This wasn't immediately obvious to me, but for anyone wondering the reason invokeMethod is used (rather than a direct call or emit) is for thread safety reasons.
Source/Core/DolphinQt/GBAHost.cpp, line 42 at r15 (raw file):
{ auto core_ptr = m_core.lock(); if (!core_ptr->IsStarted())
Should we check or assert that m_core is still valid (or equivalently, that core_ptr is not null)?
Source/Core/DolphinQt/GBAWidget.cpp, line 233 at r15 (raw file):
else { move(x() - (frameGeometry().width() / (m_local_pad & 1 ? -2 : 2)),
% 2 != 0? not sure I understand the check here
Source/Core/DolphinQt/GBAWidget.cpp, line 234 at r15 (raw file):
{ move(x() - (frameGeometry().width() / (m_local_pad & 1 ? -2 : 2)), y() - (frameGeometry().height() / (m_local_pad & 2 ? -2 : 2)));
and here
Source/Core/DolphinQt/GBAWidget.cpp, line 262 at r15 (raw file):
void GBAWidget::contextMenuEvent(QContextMenuEvent* event) { QMenu* menu = new QMenu(this);
these can be auto too
Source/Core/DolphinQt/GBAWidget.cpp, line 409 at r15 (raw file):
} GBAWidgetController::GBAWidgetController()
GBAWidgetController::GBAWidgetController() = default;
Source/Core/DolphinQt/Config/GamecubeControllersWidget.cpp, line 29 at r14 (raw file):
static const std::vector<std::pair<SerialInterface::SIDevices, const char*>> s_gc_types = { {SerialInterface::SIDEVICE_NONE, "None"},
These strings should be marked as translatable manually with _trans, like this: _trans("None")
(String literals that are passed to tr() are automatically marked as translatable, but here you need to do this manually by using the macro.)
Source/Core/DolphinQt/Config/GamecubeControllersWidget.cpp, line 117 at r14 (raw file):
{ const SerialInterface::SIDevices si_device = FromGCMenuIndex(box->currentIndex()); m_gc_buttons[i]->setEnabled(si_device != SerialInterface::SIDEVICE_NONE &&
nit: might be worth making a function for this check? It's repeated 3 times ^^
|
(Used Reviewable because it was getting difficult to track changes across pushes with GitHub's web interface. Even 4 years later the built-in review tools are still mostly unusable for large PRs.) |
There was a problem hiding this comment.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @leoetlino)
Source/Core/DolphinQt/GBAHost.cpp, line 28 at r15 (raw file):
Previously, leoetlino (Léo Lam) wrote…
This wasn't immediately obvious to me, but for anyone wondering the reason invokeMethod is used (rather than a direct call or emit) is for thread safety reasons.
I wasn't aware of QueueOnObject at the time, I've replaced those invokeMethod to use that instead to be more consistent with the rest of the code base
Source/Core/DolphinQt/GBAHost.cpp, line 42 at r15 (raw file):
Previously, leoetlino (Léo Lam) wrote…
Should we check or assert that m_core is still valid (or equivalently, that core_ptr is not null)?
This is called directly by the core itself so it can't happen. I've added the check anyway
Source/Core/DolphinQt/GBAWidget.cpp, line 233 at r15 (raw file):
Previously, leoetlino (Léo Lam) wrote…
% 2 != 0? not sure I understand the check here
The point was to arrange the windows in a nice square pattern on the first run but Qt does a good enough job on its own so I'll just let Qt handle it and remove this entirely
Source/Core/DolphinQt/GBAWidget.cpp, line 234 at r15 (raw file):
Previously, leoetlino (Léo Lam) wrote…
and here
Done.
Source/Core/DolphinQt/GBAWidget.cpp, line 262 at r15 (raw file):
Previously, leoetlino (Léo Lam) wrote…
these can be
autotoo
Done.
Source/Core/DolphinQt/GBAWidget.cpp, line 409 at r15 (raw file):
Previously, leoetlino (Léo Lam) wrote…
GBAWidgetController::GBAWidgetController() = default;
Done.
Source/Core/DolphinQt/Config/GamecubeControllersWidget.cpp, line 29 at r14 (raw file):
Previously, leoetlino (Léo Lam) wrote…
These strings should be marked as translatable manually with
_trans, like this:_trans("None")(String literals that are passed to tr() are automatically marked as translatable, but here you need to do this manually by using the macro.)
Done.
Source/Core/DolphinQt/Config/GamecubeControllersWidget.cpp, line 117 at r14 (raw file):
Previously, leoetlino (Léo Lam) wrote…
nit: might be worth making a function for this check? It's repeated 3 times ^^
Done.
There was a problem hiding this comment.
Reviewed 1 of 85 files at r19, 85 of 85 files at r20.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @Bonta0)
|
A feature I never thought would be... is. |
This replaces the current GBA devices implementation that relied on VBA-M connections and fully emulates them with libmgba instead. This one has multiple advantages: it works, it's deterministic, more convenient and makes it trivial to support SaveStates, NetPlay and Movies for those devices
New GBA profiles:
NetPlay: Device type can be configured where you would normally assign controllers
The host has the option to make players see only GBAs that correspond to a local pad since most games will expose secrets to the players on the GBA screen
Additional settings:
mGBA was added as a git submodule unlike other libraries in this project. If copying the entire tree is preferred for consistency I can do it but I believe this is more appropriate. A CMake option
USE_MGBAwas added to exclude the library if desired.Rendering is only implemented in Qt.
Known Issues:
DSP HLE: Booting with multiple GBAs will fail. This is a DSP issue, the same behavior would happen with VBA-M if you could somehow connect multiple instances simultaneously. In the meantime either use DSPLLE or connect GBAs manually one by oneFinal Fantasy Crystal Chronicles: Game will randomly drop inputs when the minimap is active on the GBA. While this still uses the same inaccurate timings as the previous implementation, using more accurate ones does not fix it, I suspect that there are more inaccuracies in Dolphin's SI implementation that also need to be fixedSonic Adventure 2: Does not connect with Sonic Advance