M3U and Disc Control interface fixes

psychonic commented 3 years ago

It seems like there were mixed reports in the current/previous issues and PRs on whether or not m3u support and the disc control interface were working or not. I fixed the issues I ran into one by one and these things seem to be fixed now. Some of this may not be ideal as I'm not hugely familiar with the Dolphin code, and am also still wrapping my head around libretro's disc control interface, but the results are there.

M3u support

Fixed by added m3u to list of supported extensions, and then normalizing slash in path to m3u before feeding it to Dolphin's path split function (which expects to only see forward slashes). Previously, m3u could not be loaded, and when they were, would have incorrect absolute paths calculated for the contents in most cases on Windows.

Disc Control

There was a previous PR for disc control, but I'm not exactly sure what it did functionally. The callbacks had basic implementations, but until the additional changes, none of the discs from the m3u were populating into the selectable indexes.

Other

Fixes #97 Also relates to #182, but I didn't test that specific case. Instead of manually adding discs, however, switching to others in the m3u does seem to work. Edit: Fixes #182. See below

bslenul commented 3 years ago

Hey! I tried your PR with my issue (#182) and it now works like a charm! Default index is now properly set to "1" instead of "null" and disc append works fine. M3u is a great addition as well and works fine too :D

So a big thank you for this! 👍

psychonic commented 3 years ago

Hey! I tried your PR with my issue (#182) and it now works like a charm! Default index is now properly set to "1" instead of "null" and disc append works fine. M3u is a great addition as well and works fine too :D

So a big thank you for this! 👍

Great! Thanks for the feedback.

bslenul commented 3 years ago

I was going to try it on my Linux VM but I get errors while compiling:

/home/bobby/Documents/dolphin/Source/Core/Core/Boot/Boot.cpp: In static member function ‘static std::unique_ptr<BootParameters> BootParameters::GenerateFromFile(std::vector<std::__cxx11::basic_string<char> >, const std::optional<std::__cxx11::basic_string<char> >&)’:
/home/bobby/Documents/dolphin/Source/Core/Core/Boot/Boot.cpp:144:13: error: ‘fs’ does not name a type; did you mean ‘ffs’?
  144 |   constexpr fs::path::value_type os_separator = fs::path::preferred_separator;
      |             ^~
      |             ffs
/home/bobby/Documents/dolphin/Source/Core/Core/Boot/Boot.cpp:145:17: error: ‘os_separator’ was not declared in this scope
  145 |   static_assert(os_separator == DIR_SEP_CHR || os_separator == '\\', "Unsupported path separator");
      |                 ^~~~~~~~~~~~

psychonic commented 3 years ago

I was going to try it on my Linux VM but I get errors while compiling:

Whoops. Should be fixed with the latest commit. That path normalization only needs to run for Windows. The declarations needed for it were gated off for that, but the actual code was not. (Technically it's gated to MSVC, but so are many other parts of Dolphin, so I doubt mingw is supported anyway).

bslenul commented 3 years ago

Thanks for the quick fix, builds fine now, and works fine (tested both append and m3u)!

bslenul commented 3 years ago

Just to be clear: the slash thing in path was only an issue for m3u, right? Sorry if it's a dumb question, I'm not a dev :D

I'm asking because on Windows when you don't use m3u, the appended disc still shows backslashes in the Current Disc Index menu:

But it seems to be completely harmless, I was able to switch between discs in MGS multiple times without any problem. Just making sure!

psychonic commented 3 years ago

Hmm. Yeah, in this case it was only an issue in the m3u handling, but there should still be consistency.

I'll do another pass a later today to make the conversions cleaner and applied in the proper direction everywhere so that RetroArch only sees the original path and Dolphin only sees the normalized path, in all places. It's probably indeed fine, but there could be other implications that we're not seeing.

psychonic commented 3 years ago

@bslenul could you do another test with the latest when you get a chance? It seems to still be working alright for me, but I'd love a larger sample size than 1.

bslenul commented 3 years ago

Working great for me too! Tested on both Windows 10 and my Linux VM, both append and .m3u, no issues! On Windows the paths in Current Disc Index are more consistent and only show backslashes now (append and .m3u)!

Thank you again for this! 👍

psychonic commented 3 years ago

Working great for me too! Tested on both Windows 10 and my Linux VM, both append and .m3u, no issues! On Windows the paths in Current Disc Index are more consistent and only show backslashes now (append and .m3u)!

Thank you again for this! 👍

Thanks again for testing again.

libretro / dolphin