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

Improve performance and UX of rom installation when handling large files #4

Merged
merged 17 commits into from
Jan 11, 2024
Merged

Improve performance and UX of rom installation when handling large files #4

merged 17 commits into from
Jan 11, 2024

Conversation

alextiley
Copy link
Contributor

@alextiley alextiley commented Jan 6, 2024

Hello, I hope you don't mind me raising this without a respective issue logged against it. Please let me know if you have a specific pull request template, and I'll be happy to switch things up.

I have been using this extension for a little while now, and it's served me well so far, so thanks for the great effort you've put in so far!

I recently decided to try and configure EmuLibrary with some roms that are quite large in file size (e.g. game dumps larger than ~5GB). During testing, I noticed that copy operations for larger files is very slow. One file which weighs in at 7.3GB took ~1.5 hours to transfer from my Synology NAS to my local machine. Copying it manually through Windows is significantly quicker, and takes around 1 minute and 30 seconds. I also wasn't sure if the copy was actually copying or not, as the install action does not currently update anything in the Playnite UI to let the user know what's happening. I could see that the file stream was being opened when looking at the destination directory, but it would sit there at zero bytes for a very long time 😅

I did some thinking and came up with some options on how to improve this:

  1. Show a notification in Playnite to notify when install starts / ends.
  2. Look into showing the Playnite global progress loading bar, and update it for each file. This is not an ideal approach (nor accurate) - directories with many files (e.g. PS3 decrypted rom folders) can vary in file size.
  3. Use the native Windows copy somehow?

After more reading I came to the conclusion that the neatest option would be to defer the copy to Windows. It seems illogical to reinvent all of this when Windows does such a good job of it already. I forked your repository and had a stab at implementing it. It works well for my use case and installs are now speedy for large files. To me it makes sense for this to go back into the main library so that others can benefit from it.

This pull request does the following things:

  • Re-writes the FileCopy util to instead use the built in OS level copying mechanism.
    • Single and multi file installs will now both use this util (the single file install controller previously had it's file copying logic hardcoded).
    • Calling the file or folder CopyAsync methods will now trigger the Windows copy prompt, which will display a progress bar and allow the user to either pause or cancel the installation.
    • Upon prompt user closure, an exception is raised, which we hook into in the respective install controller to ensure that the existing error handling does not log an error notification in Playnite. We also clean up any created top level directories that Windows may have created during copy.
  • Single and multi-file installs: updates the game "installing" status back to the default (false) in the event that install operations result in failure for some reason. This ensures that the "Install" button is rendered in Playnite after a failed install.
  • Multi-file uninstalls: fixes a minor bug where the primary rom file within the install directory was being deleted, but not the whole folder. The uninstall controller will now remove the entire folder.
  • Single and multi-file uninstalls: improves the user experience if the rom can no longer be found when triggering the uninstall controller: the game is now marked as uninstalled regardless of whether the controller was able to delete the files or not. This ensures that the game installation state is kept up to date. A message is displayed to the user in the event that this edge case arises.
  • Clears the list of roms upon uninstalling either single or multi file library items. The rom is no longer in the linked location, so it makes sense to clean it up.

Since this change, I'm now seeing install speeds on par with standard Windows copy commands (as is to be expected, since Windows is performing the copy now). What's more, the user experience is a lot more intuitive, as demonstrated in the video below:

ezgif com-video-to-gif-converter (2)

It's also now possible to pause the install, or cancel it, as demonstrated below:

ezgif com-video-to-gif-converter (1)

I have tested this change on Windows 11, but I do not have access to Windows 10 at present. I believe the methods in Microsoft.VisualBasic.FileIO are safe to use, but as C# is not my primary language of choice, please feel free to guide me if there's a better alternative.

Thanks for your time!

@alextiley alextiley changed the title feat: improve user experience of rom installation feat: improve installation of roms with large file sizes Jan 7, 2024
@alextiley alextiley changed the title feat: improve installation of roms with large file sizes Improve performance and UX of rom installation when handling large files Jan 7, 2024
@psychonic
Copy link
Owner

First, thank you for the interest, the write-up, and the proposed changes!

Re-writes the FileCopy util to instead use the built in OS level copying mechanism.

Until a better way of handling is available, I think the main piece of this change would be great as optional. While it provides a useful UI not previously present, that UI is not without it's own new issues. At least from the screenshots, there doesn't seem to be anything that is associating that copy dialog with Playnite. And in fullscreen mode, I suspect the popup would either be hidden behind Playnite or steal focus from it. If the former, then interaction with Playnite would also become effectively blocked if the user's primary input happens a controller without mouse emulation.

I do love the rest of the included bug fixes.

I hadn't realized that the file copy operation was so much slower than it could otherwise be, but even doing some brief research seems to validate that 😐. I knew it has felt sluggish as times, but hadn't dug into that. While it wouldn't solve anything with UI, I think just replacing the manual file copying with calls to File.Copy (with the whole install wrapped in Task.Run) would opt-in to most of the speed improvements, as it just wraps the underlying native API in the OS.

As for improving the visibility of pending installs, I have an ambitious plan that I may or may not ever actually get to, mostly because I'm still a beginner when it comes to WPF. My thought was to use Playnite's sidebar support to show global progress (to not block usage of the actual global progress bar, but still with the caveats of trying to figure out what the total progress of an install actually is). And then, with or without that showing accurate progress, clicking it could go to a new window with UI to show the in-progress or recent installs, with some details and cancel button, etc.

image

Show a notification in Playnite to notify when install starts / ends.

This is already implemented, but as optional and only for install end currently.

I believe the methods in Microsoft.VisualBasic.FileIO are safe to use

I believe that's the case at well, at least for .NET Framework / Playnite 10. I don't know offhand if that's available on newer .NET versions, but I'd be surprised if no one reimplemented it even if Microsoft themselves didn't.

@alextiley
Copy link
Contributor Author

alextiley commented Jan 7, 2024

Thanks for coming back to me on this, and appreciate my PR description was quite lengthy so sorry about that!

...in fullscreen mode, I suspect the popup would either be hidden behind Playnite or steal focus from it. If the former, then interaction with Playnite would also become effectively blocked if the user's primary input happens a controller without mouse emulation.

I somehow managed to forget the Playnite had a fullscreen mode! I've just checked this, and you're right that the popup steals focus (but it does display). I guess this is similar to how any other install would work for non EmuLibrary games, but I agree with your point and accept that accessibility would suffer.

I do love the rest of the included bug fixes.

Great! Thinking about it, it probably makes sense to split those out to separate PRs, so I'll have a look at doing that sometime this week.

I hadn't realized that the file copy operation was so much slower than it could otherwise be.

Yes, this was the main motivation for the change to be honest. I'm personally not too worried about the progress updates via the popup, but given that it can be slow for large copies, it prompted me to think about how I could see what was going on.

While it wouldn't solve anything with UI, I think just replacing the manual file copying with calls to File.Copy (with the whole install wrapped in Task.Run) would opt-in to most of the speed improvements

Yes I think this should work and happy to repurpose what I've got here to cover this.

Until a better way of handling is available, I think the main piece of this change would be great as optional.

From your comments, I'm wondering if you would consider support for two copy strategies then?

  1. Using File.Copy, replacing the existing implementation, which should improve performance. This could be enabled by default and should provide an uplift in transfer speeds, making install of larger roms less confusing.
  2. Filesystem.CopyFile / FileSystem.CopyDirectory (with popup, as implemented in this PR), which could be enabled in the library config screen via some new checkboxes, e.g. "Use Windows copy dialog in desktop mode" and "Use Windows copy dialog in fullscreen mode".

Let me know your thoughts, and thanks again for the feedback!

@psychonic
Copy link
Owner

psychonic commented Jan 7, 2024

From your comments, I'm wondering if you would consider support for two copy strategies then?

  1. Using File.Copy, replacing the existing implementation, which should improve performance. This could be enabled by default and should provide an uplift in transfer speeds, making install of larger roms less confusing.
  2. Filesystem.CopyFile / FileSystem.CopyDirectory (with popup, as implemented in this PR), which could be enabled in the library config screen via some new checkboxes, e.g. "Use Windows copy dialog in desktop mode" and "Use Windows copy dialog in fullscreen mode".

I think this sounds great! It should allow for faster copying in every scenario, while still allowing the user to choose which UX that they prefer without forcing them into any change.

Thinking about it, it probably makes sense to split those out to separate PRs, so I'll have a look at doing that sometime this week.

I agree that distinct PRs are preferable and more clean, but I recognize that it would also require at least some more effort on your part. With us being in sync now with the larger change, I'll leave it up to you. I'm not overly concerned about it, and am happy to have the contributions regardless.

Thanks again

@alextiley
Copy link
Contributor Author

alextiley commented Jan 10, 2024

Hey @psychonic, this is now ready for review again! I also made some minor changes to the layout of the settings UI - see screenshot below. It handles text wrapping for smaller windows (and handheld screens).

image

I've implemented and tested scenarios for installing in both desktop and fullscreen modes with the new options turned on and off - behaviour all looks good to me. Rom installs are now way faster. Let me know if there's anything you'd like to tweak.

<StackPanel x:Name="EmulatorSettingsPanel">
<Label Content="Emulator path mappings" FontWeight="Bold" />
<TextBlock HorizontalAlignment="Right" Margin="0,-16,0,0">
<Hyperlink NavigateUri="https://github.com/psychonic/Playnite-EmuLibrary/blob/master/README.md#setup" RequestNavigate="Hyperlink_RequestNavigate">
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 also noticed that this link didn't work, so I've pointed it at the correct README.md URL.

Copy link
Owner

Choose a reason for hiding this comment

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

Good catch

@psychonic
Copy link
Owner

This is perfect. Thanks again!

@psychonic psychonic merged commit 4e8183f into psychonic:master Jan 11, 2024
@alextiley alextiley deleted the feature/progress-bar-during-installs branch January 11, 2024 07:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants