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

Add /dev/dolphin for homebrew to get information about Dolphin #8445

Merged
merged 1 commit into from Nov 10, 2019

Conversation

@Leseratte10
Copy link
Contributor

Leseratte10 commented Nov 1, 2019

I added some code to Dolphin, mainly a new device /dev/dolphin that can be used by Dolphin-aware software like homebrew to query certain information about Dolphin.

Currently, that includes the real system time in ms (not the emulated system time), and the Dolphin version. Now I know from past conversations on the forum that there is no way for emulated software to detect real system time as that might confuse games when system time and emulated time run at different speeds.

However, implementing this as an additional device /dev/dolphin means that no official game will ever know of this, as no official game would ever access /dev/dolphin.

For Wiimmfi it would be pretty cool to let the game know that it lags (doesn't run at full 60fps), by comparing emulated time with this real time. Currently, when a player plays Mario Kart Wii online, they get a huge advantage when their Dolphin doesn't run at 60fps all time. If the game knew exactly how much it lags (that's the information that this PR gives the game) it can adapt the current racing timer, making that run at real-time. That would remove the advantage playing at <60fps gives you, and would allow players with slower computers to play on Wiimmfi as well.

Currently still a work in progress as I need to test it some more and might want to add another thing to the /dev/dolphin interface, but I wanted to create this PR to kinda ask the Dolphin team if there is a possibility for getting this merged at some point or if you'd have any objections against such an interface.

@Antidote

This comment has been minimized.

Copy link

Antidote commented Nov 2, 2019

I think this is a good idea, especially if it gets fleshed out more.

@JMC47

This comment has been minimized.

Copy link
Contributor

JMC47 commented Nov 2, 2019

I'm not against this, it's a pretty nifty idea and it could potentially help with cheaters and unintentional disruptions from users lagging on games like Mario Kart Wii.

Source/Core/Core/IOS/DolphinDevice.cpp Outdated Show resolved Hide resolved
Source/Core/Core/IOS/DolphinDevice.cpp Outdated Show resolved Hide resolved
Source/Core/Core/IOS/DolphinDevice.h Outdated Show resolved Hide resolved
Source/Core/Core/IOS/DolphinDevice.cpp Outdated Show resolved Hide resolved
Source/Core/Core/IOS/DolphinDevice.h Outdated Show resolved Hide resolved
Source/Core/Core/IOS/DolphinDevice.h Outdated Show resolved Hide resolved
Source/Core/Core/IOS/DolphinDevice.h Outdated Show resolved Hide resolved
Source/Core/Core/IOS/DolphinDevice.cpp Outdated Show resolved Hide resolved
@Leseratte10 Leseratte10 force-pushed the Leseratte10:master branch from 66d0f39 to 422b911 Nov 2, 2019
@Leseratte10

This comment has been minimized.

Copy link
Contributor Author

Leseratte10 commented Nov 2, 2019

Just added the changes suggested by lioncash in the commit 422b911. I wasn't exactly sure how "This can likely be replaced with forward declarations." was meant, but I noticed that two of these included files weren't even needed and removed the include for these.

Source/Core/Core/IOS/DolphinDevice.cpp Outdated Show resolved Hide resolved
Source/Core/Core/IOS/DolphinDevice.cpp Outdated Show resolved Hide resolved
@Leseratte10 Leseratte10 force-pushed the Leseratte10:master branch from 422b911 to 43e8d49 Nov 2, 2019
@Leseratte10

This comment has been minimized.

Copy link
Contributor Author

Leseratte10 commented Nov 2, 2019

New commit 43e8d49, missing includes added (algorithm & cstring), unused include removed (sys/time.h, that was from before I knew about Common/timer.h), and enum moved to CPP file.

@BhaaLseN

This comment has been minimized.

Copy link
Member

BhaaLseN commented Nov 2, 2019

You should consider running clang-format on your code; having it look and feel the same in every file helps by presenting a consistent style where everything looks familiar at some point.
Also, I don't see any vcxproj/filters changes, so this wont work on Windows as-is.

@Leseratte10 Leseratte10 force-pushed the Leseratte10:master branch 2 times, most recently from 02bdb89 to a0b9928 Nov 2, 2019
@Leseratte10

This comment has been minimized.

Copy link
Contributor Author

Leseratte10 commented Nov 2, 2019

Ran clang-format and added the new files to the vcxproj files, didn't know Windows used something other than the CMake lists. Hoping this works on Windows now, I don't have a Windows environment to test this.

@BhaaLseN

This comment has been minimized.

Copy link
Member

BhaaLseN commented Nov 2, 2019

Buildbot seems to be fine with that change. We're still looking into using CMake on Windows as well, but that is still some time out until its ready.

@delroth

This comment has been minimized.

Copy link
Member

delroth commented Nov 2, 2019

Should probably be disabled in determinism mode just in case.

I very much like this change. One use case I have for this is when writing PPC tests for Dolphin, it could give us a nice interface to report test outputs.

@Leseratte10

This comment has been minimized.

Copy link
Contributor Author

Leseratte10 commented Nov 2, 2019

So that determinism mode is for stuff like Netplay where the exact same output is preferred, right? So I could just make it return an error instead ot system time when in determinism mode. Or can that mode be enabled during online play as well and mess that up?
Would the version reporting need to be disabled as well?

@delroth

This comment has been minimized.

Copy link
Member

delroth commented Nov 2, 2019

I would just not expose the device in this condition really. It's supposed to be a general interface for host access, which by nature will be 99% non-deterministic (the version number thing is likely the exception).

If I'm correct it should only be enabled during netplay and input recording / TAS, but there might be other conditions.

@JosJuice

This comment has been minimized.

Copy link
Contributor

JosJuice commented Nov 2, 2019

So that determinism mode is for stuff like Netplay where the exact same output is preferred, right?

Yes.

Or can that mode be enabled during online play as well and mess that up?

Connecting to the internet is disabled when using deterministic mode, since the data you receive from servers on the internet isn't deterministic.

Would the version reporting need to be disabled as well?

Having version reporting enabled when using deterministic mode isn't a problem in practice since we don't guarantee determinism across different versions of Dolphin. But it might make sense to just not expose the device at all when using deterministic mode.

@Leseratte10 Leseratte10 force-pushed the Leseratte10:master branch from a0b9928 to e98b879 Nov 2, 2019
@Leseratte10

This comment has been minimized.

Copy link
Contributor Author

Leseratte10 commented Nov 2, 2019

Done. /dev/dolphin is now only exposed when Core::WantsDeterminism() is false.

@JosJuice

This comment has been minimized.

Copy link
Contributor

JosJuice commented Nov 2, 2019

Hmm... There's also the case where determinism mode is enabled after emulation starts, though. (This only happen when starting a TAS from a savestate.) Is it possible to remove a device after it has been added?

@Leseratte10

This comment has been minimized.

Copy link
Contributor Author

Leseratte10 commented Nov 2, 2019

I don't know. There is a function AddDevice, but I don't see RemoveDevice. And that might cause problems when you remove a device while it's opened by the game?
Maybe it'd be better to just expose it all the time but then return IPC_EINVAL or IPC_EACCES when it is accessed in determinism mode.

@Leseratte10 Leseratte10 force-pushed the Leseratte10:master branch from e98b879 to 59986f6 Nov 2, 2019
@Leseratte10

This comment has been minimized.

Copy link
Contributor Author

Leseratte10 commented Nov 2, 2019

/dev/dolphin is now created all the time, but DolphinDevice::IOCtlV checks Core::WantsDeterminism() and returns IPC_EINVAL if that is true.

@Leseratte10

This comment has been minimized.

Copy link
Contributor Author

Leseratte10 commented Nov 2, 2019

In case you are interested, this is what it will look when this patch is added to Dolphin and the necessary server changes are rolled out to everyone: https://www.youtube.com/watch?v=NKtpx6g8TNg

This is a live view of a Mario Kart Wii race, with Dolphin's speed limit set to 30%. The players are jumping all over the place, but the timer in the top right runs at real time, and the advantage players were getting with a slow Dolphin is gone.

@Antidote

This comment has been minimized.

Copy link

Antidote commented Nov 2, 2019

One suggestion @jackoalan made for an addition to this is the ability to print directly to dolphin without the need to patch OSReport or printf

It was also suggested to add a pseudo terminal to the debug UI (not within the scope of this PR) where these messages are printed to without being burdened by other useful log messages in the log viewer.

And this is probably far outside the scope, but the ability to send and receive commands between dolphin and the running application for a REPL interface would also be incredibly useful, but could definitely be handled in a separate PR if this gets merged.

@Leseratte10 Leseratte10 changed the title [WIP] Add /dev/dolphin for homebrew to get information about Dolphin Add /dev/dolphin for homebrew to get information about Dolphin Nov 2, 2019
@Leseratte10

This comment has been minimized.

Copy link
Contributor Author

Leseratte10 commented Nov 2, 2019

You mean to be able to print to the Dolphin log without Dolphin having to intercept the output of OSReport? What would be the point when the current solution works? Or is there some situation where the current process fails?

Might not be a bad idea to have a seperate terminal for debug output from own custom code that doesn't go into the Dolphin log, but I agree with you that that should be done in a seperate PR.
As visible in the video above, this code seems to work so far, so I removed the "WIP" prefix from the PR. I'll get some more people to test with this version with the beta server code enabled.

@Antidote

This comment has been minimized.

Copy link

Antidote commented Nov 2, 2019

The patching method can be unreliable, I've had issues with my own homebrew where printf was not properly hooked and I was unable to use the built in log, having a direct route to Dolphin without a potentially failed hook is desireable imho. I think having the hooks works extremely well for games using the official SDK, but it's hit or miss in my personal experience developing homebrew.

A further anecdote is when I was attempting to develop a Wii version of my skyward sword save editor and I could not get dolphin to reliably print any of my debug messages and I wound up having to sacrifice my framebuffer to print them that way.

@Leseratte10 Leseratte10 force-pushed the Leseratte10:master branch from 59986f6 to bb0caef Nov 2, 2019
@Leseratte10 Leseratte10 force-pushed the Leseratte10:master branch from c6cf28b to edf945b Nov 4, 2019
@foolishsyrup

This comment has been minimized.

Copy link

foolishsyrup commented Nov 8, 2019

What is the status of this?

@JMC47

This comment has been minimized.

Copy link
Contributor

JMC47 commented Nov 8, 2019

Probably just waiting on reviews. Everyone seems in favor of this, it's just getting people to review/approve it so it can be merged.

@Leseratte10

This comment has been minimized.

Copy link
Contributor Author

Leseratte10 commented Nov 8, 2019

I put it back in WIP status because I'm trying to find a way to get rid of the lag start (happens when Dolphin runs too slow during track selection) as well. I didn't get around to implementing the necessary server changes yet (will probably do so on the weekend).

Most likely that will not require any other changes to Dolphin, but to make sure I'd like to leave this PR open until this is done so in case it does need more changes to Dolphin these can be done in this PR and I don't have to open another one.

I'll let you know when the server changes are ready (and if what I'm planning for the track selection even works).

@Leseratte10

This comment has been minimized.

Copy link
Contributor Author

Leseratte10 commented Nov 8, 2019

I just hacked the fix for the track load slowdown into my game manually (not yet rolled out on Wiimmfi yet) and it seems to work. Doesn't get rid of the slowdown completely, but helps a bit.

So as soon as the latest changes get reviewed / approved by someone this PR will be ready to be merged.

@Leseratte10 Leseratte10 changed the title [WIP] Add /dev/dolphin for homebrew to get information about Dolphin Add /dev/dolphin for homebrew to get information about Dolphin Nov 8, 2019
@JosJuice

This comment has been minimized.

Copy link
Contributor

JosJuice commented Nov 8, 2019

I'm a bit sceptical about letting the emulated software override the user's emulation speed like that... If we're going to do it, we should at least do it for the current session only (which I unfortunately suppose would be a bit hacky to implement since the emulation speed hasn't been migrated to the new config system).

@Leseratte10

This comment has been minimized.

Copy link
Contributor Author

Leseratte10 commented Nov 8, 2019

I tested that, and if I remember correctly it did only override the speed for the current session (or until the user changes it in the config again). I can test that again to make sure if you want.

@JosJuice

This comment has been minimized.

Copy link
Contributor

JosJuice commented Nov 8, 2019

It's possible that I'm misunderstanding the BootManager logic and that it in fact is already is for the current session only. If you want to re-test it, I would appreciate it.

@Leseratte10

This comment has been minimized.

Copy link
Contributor Author

Leseratte10 commented Nov 8, 2019

Oh, damn, it looks like this is indeed not being reset. The settings menu shows 100% again, but even after restarting Dolphin the speed is still set to the value the game set it to (and it's displayed wrong in the settings), until I actively set it to something else and then back to 100% in the settings. Even though this setting will only be used temporarily and will set the speed back to 100% later, that will cause problems if someone exits the game or quits Dolphin while in a higher speed mode.

Any idea how this could be implemented for the current session only?

@JosJuice

This comment has been minimized.

Copy link
Contributor

JosJuice commented Nov 8, 2019

You'll probably want to set bSetEmulationSpeed in BootManager.cpp to true, or do something else involving ConfigCache in BootManager.cpp. The problem is how to do this while keeping the interface clean...

@Leseratte10

This comment has been minimized.

Copy link
Contributor Author

Leseratte10 commented Nov 8, 2019

So I'd probably need to add a public function in BootManager.cpp that sets the bSetEmulationSpeed in the private struct?

@Leseratte10 Leseratte10 force-pushed the Leseratte10:master branch from edf945b to 55d05bb Nov 8, 2019
Source/Core/Core/BootManager.cpp Outdated Show resolved Hide resolved
Source/Core/Core/IOS/DolphinDevice.cpp Outdated Show resolved Hide resolved
Source/Core/Core/IOS/DolphinDevice.cpp Outdated Show resolved Hide resolved
Source/Core/Core/IOS/DolphinDevice.cpp Outdated Show resolved Hide resolved
Source/Core/Core/IOS/DolphinDevice.cpp Outdated Show resolved Hide resolved
Source/Core/Core/IOS/DolphinDevice.cpp Outdated Show resolved Hide resolved
@Leseratte10 Leseratte10 force-pushed the Leseratte10:master branch 2 times, most recently from 283ab2a to 6a381fd Nov 9, 2019
@fancythedeveloper

This comment has been minimized.

Copy link

fancythedeveloper commented Nov 9, 2019

Well, hope this PR gets merged. Definitely will help Dolphin players play Wiimmfi w/o having to worry about Dolphin lagging (from dropping frames).

Source/Core/Core/IOS/DolphinDevice.cpp Outdated Show resolved Hide resolved
Source/Core/Core/IOS/DolphinDevice.cpp Outdated Show resolved Hide resolved
Source/Core/Core/IOS/DolphinDevice.cpp Outdated Show resolved Hide resolved
@Leseratte10 Leseratte10 force-pushed the Leseratte10:master branch 2 times, most recently from 7570b58 to 77d7caf Nov 9, 2019
Source/Core/Core/IOS/DolphinDevice.h Show resolved Hide resolved
Source/Core/Core/BootManager.cpp Outdated Show resolved Hide resolved
Adds a /dev/dolphin interface that can be used by
Dolphin-aware software to get information like the
real system time and the Dolphin version.
@Leseratte10 Leseratte10 force-pushed the Leseratte10:master branch from 77d7caf to 2d55a6b Nov 9, 2019
Copy link
Member

leoetlino left a comment

Implementation LGTM.

Emulation speed overrides not being clearly indicated in the UI is an existing issue, and one that will get solved as we continue migrating to the new config system, so I think that shouldn't be a blocker.

@fancythedeveloper

This comment was marked as off-topic.

Copy link

fancythedeveloper commented Nov 10, 2019

someone better merge this now or one kid dyin' in the morn

@Leseratte10

This comment has been minimized.

Copy link
Contributor Author

Leseratte10 commented Nov 10, 2019

So now that this has been approved by @leoetlino what is the process to actually get this merged and into Dolphin? Will someone else need to review this PR as well? From my side this should now be done, all the other changes will now be server-side and not in Dolphin.

@JosJuice

This comment has been minimized.

Copy link
Contributor

JosJuice commented Nov 10, 2019

So now that this has been approved by @leoetlino what is the process to actually get this merged and into Dolphin? Will someone else need to review this PR as well?

Basically, yes. There's no exact process that's set in stone, but that's how it often works. Either way, there's nothing more you need to do with this PR for now unless someone leaves a comment about something to change.

@delroth delroth merged commit df32e3f into dolphin-emu:master Nov 10, 2019
10 checks passed
10 checks passed
default Very basic checks passed, handed off to Buildbot.
Details
lint Build succeeded on builder lint
Details
pr-android Build succeeded on builder pr-android
Details
pr-deb-dbg-x64 Build succeeded on builder pr-deb-dbg-x64
Details
pr-deb-x64 Build succeeded on builder pr-deb-x64
Details
pr-freebsd-x64 Build succeeded on builder pr-freebsd-x64
Details
pr-osx-x64 Build succeeded on builder pr-osx-x64
Details
pr-ubu-x64 Build succeeded on builder pr-ubu-x64
Details
pr-win-dbg-x64 Build succeeded on builder pr-win-dbg-x64
Details
pr-win-x64 Build succeeded on builder pr-win-x64
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.