Track Time Played (Core and QT) #11469
Conversation
There was a problem hiding this comment.
@dolphin-emu-bot rebuild
I like the idea in general; and we could explore a future extension of this to also work for Wii games and write their playtime to the NAND (so they show up in the Wii menu).
Also, your commits are currently structured in a way that they reflect your personal TODO list and/or progression, which is not too useful for us. We'd prefer one (or more) commits that are self-contained, build in isolation and have a commit message that explains their intent.
In this case, one commit is probably good enough, and should be easily possible as follows:
To squash those commits into one before continuing; you can use the following commands (replace upstream with the name of the Dolphin remote and origin with the name of your Fork remote):
git fetch upstream
git reset --soft upstream/master
git commit -m "your commit message here"
git push -f origin HEAD
The first command makes sure you get the latest commits from master (from the Dolphin repository) to work on; the second picks all your changes, switches to that master state and puts the changes in the staging area. The third command then commits it so you have one commit total that has all the changes (which you can also replace with your commit method of choice, such as git gui or any other GUI tool); and the fourth command pushes your current branch into your own remote (which should be your Fork) while overwriting all previous changes (to update this PR).
|
This needs to be made translatable. https://doc.qt.io/qt-6/i18n-source-translation.html#using-numbered-arguments |
|
So design wise, but I'm concerned about the amount of horizontal space the time played takes in that screenshot. I would definitely prefer if it was in a shortened format like this - "6h 8m". It still conveys the information but it takes WAY less space. |
|
I squashed the commits again though the language files seemed to be automatically modified |
|
It looks as if you rebased this pull request on the latest version of master but then undid the changes from the last version of master. EDIT: You undid more than just the latest commit, actually. See the change to State.cpp, for instance. |
|
The |
|
@BhaaLseN Hold on, why is there a soft reset in the instructions you posted? I'm pretty sure that's the cause of this happening. |
I did that, I followed these steps that were posted before. git fetch upstream |
The soft reset leaves changes (from this branch; not from the diff between the old and new master) in the staging area, vs. a mixed reset that leaves them unstaged. It shouldn't cause this problem (at least not by itself). I didn't check history yet, but a It might cause this when the commits this branch is based on are suddenly gone; but this would mean that master was rewritten; or that this PR was based on another one (which was rebased in the mean time) - neither of which is the case here. I'm afraid at this point, the only way to fix this is by unstaging the unintended reverts by hand, and resetting them locally. At least I can't think of a simple way to get back; other than |
When you soft reset, the state of all files is kept as they were. That is, not only are the changes from the PR preserved, but none of the files that have been modified in master are brought up to date with master. When you then commit your staged changes, you are committing your old versions of files that have been modified in master. |
Interresting, I've used this a bunch before and never had any issues. The docs are are bit ambiguous though, now that I read the section again ("doesn't touch the index" but "leaves all changed files to be committed"). Guess I'll drop this out of my saved replies for safety reasons if it has the potential to mess things up. |
|
I removed the accidental changes and re-pushed. |
|
Could you update the image in the PR description please? |
It's updated |
|
I also don't see any handling of the emulator pausing here, so paused time counts as playtime. I suspect that is not desired. |
It would not count time while in the middle of TASing a game and have the game paused, but maybe an average user expects a paused game to not count time; I'm conflicted on whether to count it or not. Edit: I decided to not add to the time when emulation is paused. |
|
@BhaaLseN wrote:
I thought this already worked on master, but it looks like it only properly works if you launch games from the system menu (otherwise you get a 23:59 blank entry). Apparently the system menu needs to do some stuff with play_rec.dat beforehand, and then games will update it. But we aren't currently creating it beforehand, so things break (and presumably we'd also need to update the message board before each start so that things behave properly if multiple games are launched without loading the system menu). As far as I can tell, the feature added by this PR works correctly for Wii games, though. |
|
I have addressed the changes requested and made the time tracking optional. Let me know if there's anything else that needs to be done. |
|
Please squash the commits. Otherwise, this seems fine enough I guess. One could definitely improve this further (eg. there's a rather large timing inaccuracy around pausing) but it'll probably do the job for now. Completely untested. |
Creates TimePlayed class and implemented constructors, AddTime, GetTimePlayed, and Reload methods. Updates CMakeLists.txt and DolphinLib.props as appropriate.
Introduce method to track the time played for a game via time differences and TimePlayed methods. Threads are synchronized via Common::Event.
All metadata access methods now acquire a lock on `m_metadata_lock` to prevent race conditions.
Shows minutes/hours in the list view and handles column visibility.
There was a problem hiding this comment.
Gave this a quick try on macOS. First of all, it works on macOS! Tried a few other quick tests too.
- Does Dolphin record time if time tracking is disabled? Test result: No. (good)
- Does Dolphin record time if time tracking is enabled but the column removed? Test result: Yes. (good)
That's very very basic testing, but yea, it works!
UX notes, I have mixed feelings about it being enabled by default, but since you added the ability to turn it off, it's fine. It would also be nice if the column updated live while a game was being played, but updating when a game is stopped is acceptable. Also all of the previous UX quibbles I had in this PR are resolved.
So yea, design LGTM. I did not review the code and only performed light testing.
aminoa
force-pushed
the
master
branch
2 times, most recently
from
f24c9b7 to
1476a92
Compare
|
Don't think I'll be able to do a throughout review in time, but when exactly is the play time saved to the user folder? What happens if Dolphin crashes during emulation, is the playtime of that session (until the crash) also gone? |
It's saved every 30 seconds to an ini file so a crash would at most lose 30 seconds too. |
Introduce a new "Enable Time Tracking" checkbox in the InterfacePane UI. The checkbox is dynamically enabled or disabled based on the emulation state, preventing changes while emulation is active.
|
FifoCI detected that this change impacts graphical rendering. Here are the behavior differences detected by the system:
automated-fifoci-reporter |
|
I'm in the middle of a more complete review but I want to prioritize this point since changing Config names later would break backward compatibility. I'd like to get feedback on this from other people, so don't make these changes until they've had a chance to respond. The config As for the name, I think something like |
This adds a total "Time Played" tab in the QT interface - the time is started at the beginning of emulation and ended when emulation shuts down. A general TimePlayed class is implemented which the desktop QT interface pulls from. This does not require a standard exit of emulation to count the time.
Edit (January 2025): A toggle has been added to allow the user to disable time tracking if desired.