libretro / dosbox-svn

GNU General Public License v2.0
6 stars 17 forks source link

Relative paths in playlist unsupported by core #69

Closed ner00 closed 3 years ago

ner00 commented 3 years ago

I've tried both DOSBox-core and DOSBox-SVN, they don't accept relative paths for files in the playlist, most if not all other cores do. Take this for example: "path": "D:\\RetroArch\\roms\\DOS\\Call of Cthulhu - Shadow of the Comet\\dosbox.conf",

Ideally, it should also work as below: "path": "roms\\DOS\\Call of Cthulhu - Shadow of the Comet\\dosbox.conf",

A simple situation where this is most useful is when you have your library on a USB drive which can have a different letter if another device has been recently plugged beforehand. Obviously it also encompass every other situation like parent folder rename and any folder structure change in general.

realnc commented 3 years ago

Just tried it. Looks like a RetroArch bug to me. The `File could not be loaded from playlist" error is generated by RetroArch before it even attempts to tell the core which playlist entry was selected.

ner00 commented 3 years ago

Yeah, maybe I should open a ticket concurrently in RetroArch repo.

ner00 commented 3 years ago

It seems that it's the core that does not translate the relative path correctly, I don't think RetroArch has a different handling of playlist paths depending on the core.

This is what I get with DOSBox in the log file with a relative path in the playlist: [INFO] [CORE]: Using content: roms\DOS\Call of Cthulhu - Shadow of the Comet\dosbox.conf.

The first thing this indicates is that the path has been handled to the core exactly in the same way as every other core, and the core starts the first time around but stops at the DOS prompt as if no config file argument was given.

If standalone DOSBox were to be used as an example, the correct relative path would have been this (assuming cores as the working dir): DOSBox.exe -conf "..\roms\DOS\Call of Cthulhu - Shadow of the Comet\dosbox.conf"

The ..\ part is mandatory for a normal executable, but I don't know how cores get past that, they must assume RetroArch.exe run dir as the working dir, something which DOSBox-core/SVN does not seem to be doing.

ner00 commented 3 years ago

One last test I made was placing the .conf file in the root of RetroArch and edit the playlist entry accordingly: "path": "dosbox.conf", this worked fine and confirmed that the DOSBox core also assumes the root folder as working dir just like other cores do.

This means that the target file is not above the working dir and the relative path should work (without the need for ..\), as it does if standalone DOSBox is used, so the problem really seems to be how the core is handling the path, which is strange because it deviates from how it would normally operate.

Also, I don't understand how RetroArch runs the core the first time but gives the File could not be loaded from playlist on subsequent tries for the same entry.

realnc commented 3 years ago

I see. It wasn't working for me because I set /tmp as the working directory in my RetroArch shortcut. (I'm on Linux.) So any relative paths in playlists were relative to /tmp.

This is only working by accident basically. What RetroArch should really do is resolve relative paths to absolute paths when passing them to the core, because only RA actually knows which directory the playlist file is in. The -core and -SVN cores change the current working directory to directory the .conf file is in, so that mount commands in .conf files can be relative to the .conf file itself. (This is for making it possible to keep .conf files together with the game itself and be able to copy the whole directory somewhere else.)

I changed DOSBox-core to convert the relative path that RA passes to an absolute path instead, so that it still works after the core changes the current working directory. Ideally though, RA should convert relative paths to absolute paths before passing them to cores, because relative paths in .lpl files should really not depend on the working directory of RA.

Not sure when I'll change DOSBox-SVN.

ner00 commented 3 years ago

Oh, okay, well thanks for the current commit and clarifying things. I'll open an issue in RA repo and reference this to try and push for that change, I don't see a reason for any resistance on this, it shouldn't impact how everything else is working at the moment.

ner00 commented 3 years ago

Not sure when I'll change DOSBox-SVN.

@realnc Sorry, quick question: is there any intricacy that makes the same workaround not applicable to DOSBox-SVN core? I'm probably being naive, but I assumed this would be it:

                if(extension == "conf")
                {
            #ifndef HAVE_LIBNX
                    configPath = std::filesystem::absolute(loadPath);
            #else
                    configPath = loadPath;
            #endif
                    loadPath.clear();
                }

https://github.com/libretro/dosbox-svn/blob/810775bc1a6dcd882eac7e8da9656350ffce3986/libretro/libretro.cpp#L1449

In any case, thank you once again for addressing this.

realnc commented 3 years ago

@realnc Sorry, quick question: is there any intricacy that makes the same workaround not applicable to DOSBox-SVN core?

DOSBox-core exists because in -SVN, modern C++ code is not allowed. The libretro people want to keep this core compatible with old systems (like Nintendo 3DS and such) that only support legacy C++. (Called "C++98.") So I created DOSBox-core instead where I could use modern C++ to implement the extra features quicker.

std::filesystem::absolute() is modern C++ so it won't compile if you put that in DOSBox-SVN.

Note that I don't actually maintain DOSBox-SVN. It's not my project. DOSBox-core is.

ner00 commented 3 years ago

@realnc I think this whole thing might have been a little premature on my part.

I just found out about Portable Playlists setting in RetroArch, which allows the user to have absolute paths in playlists; base_content_directory is added to the playlists header and is checked against rgui_browser_directory on load, if they mismatch, RetroArch will recompile the playlist with the new base_content_directory for every entry's path - essentially refreshing all absolute paths to resolve to valid destinations again.

This has been introduced in v1.9.0 back in June or July, I stumbled upon it by accident. It seems that it addresses the issue in terms of playlists at the very least. I am unsure if you were already looking at this in terms of -SVN implementation, but I probably caused you to do extra work with no need.

I'll wait for your reply before closing this, as well as other related issues that I opened and became irrelevant considering the existence of Portable Playlists setting.

realnc commented 3 years ago

I am unsure if you were already looking at this in terms of -SVN implementation

Nope, I didn't do anything related to that.