Skip to content

fix(pegasus): correct relative media paths#147

Closed
cameronhimself wants to merge 1 commit into
Gemba:masterfrom
cameronhimself:pegasus_rel_media_paths
Closed

fix(pegasus): correct relative media paths#147
cameronhimself wants to merge 1 commit into
Gemba:masterfrom
cameronhimself:pegasus_rel_media_paths

Conversation

@cameronhimself
Copy link
Copy Markdown

It looks like there was an issue when generating relative media paths for Pegasus. It was using the inputFolder config value rather than mediaFolder. As a result, if your media dir wasn't nested in your ROM input dir you'd end up with absolute paths for media files no matter relativePaths was set to.

@Gemba
Copy link
Copy Markdown
Owner

Gemba commented Apr 3, 2025

Thanks for reporting.

I still have some questions:

  1. Have you seen Lars' comment here? (my bad I have not moved it to the addMediaFile() member as I edited that file)
  2. Did you provide a -o or mediaFolder= value for the output? If yes: is the path relative or absolute?
    (if none is provided it uses the value of gameListFolder which is the the inputFolder value, unless a gamelist folder is provided (-g/gameListFolder=). The input folder in turn, if not provided defaults to $HOME/RetroPie/roms/<platform>)
  3. As this code section is a little aged: Does Pegasus now have a different mediaFolder as default?

@cameronhimself
Copy link
Copy Markdown
Author

Answers to your questions

  1. Have you seen Lars' comment here? (my bad I have not moved it to the addMediaFile() member as I edited that file)

I had not. But I don't think I understand his argument for reasons I'll explain in the next section

  1. Did you provide a -o or mediaFolder= value for the output? If yes: is the path relative or absolute?

I provided a mediaFolder value, and it's absolute. In my experience relative paths don't work at all in the ini file--it always looks in ~/.skyscraper for relative paths, which is definitely not what I expect or want. I would expect the paths to be relative to the ini file. But this is a tangent.

  1. As this code section is a little aged: Does Pegasus now have a different mediaFolder as default?

Not to my knowledge, no--it still stores everything in the game dir by default.

My use case

Here's my configuration:

[main]
relativePaths="true"
gameListFolder="/path/to/pegasus"
mediaFolder="/path/to/media"
inputFolder="/path/to/roms"

If I run skyscraper -c `pwd`/skyscraper.ini -p snes -f pegasus in /path/to I get the following game list at /path/to/pegasus/snes/metadata.pegasus.txt:

collection: Super Nintendo
shortname: snes
launch: /opt/retropie/supplementary/runcommand/runcommand.sh 0 _SYS_ snes "{file.path}"

game: Super Mario World
file: ./Super Mario World.smc
assets.screenshot: /path/to/media/snes/screenshots/Super Mario World.png
assets.boxFront: /path/to/media/snes/covers/Super Mario World.png
assets.marquee: /path/to/media/snes/marquees/Super Mario World.png
assets.wheel: /path/to/media/snes/wheels/Super Mario World.png

There are two things that seem wrong about this output:

  1. The file parameter is technically relative, but it's relative to the inputFolder, which is only useful if I haven't modified the gameListFolder. If I add the game list to Pegasus it won't be able to find the ROM. I would expect the path to be relative to the gameListFolder so Pegasus can always locate the ROM.
  2. The assets dirs are simply not relative, despite explicitly setting relativePaths to true. That's what this PR was hoping to address, though even with that change they're still relative to the wrong dir, the mediaFolder dir, and Pegasus cannot find them. Again, they should be relative to the gameListFolder so Pegasus is always able to locate them.

I guess Lars' comment was implying he wanted to avoid precisely this situation with the media files, but, if so, it's an inconsistent standard since it wasn't applied to the file param and the inputFolder/gameListFolder params.

This PR doesn't address all of these issues, obviously--it only addresses the inconsistent application of the relativePaths option. But it's evidently not causing trouble for others, so no worries if it's not considered worth changing. Maybe I'll spend some time on a more comprehensive overhaul of the relative path handling, addressing the issues outlined above, if you think that would be worthwhile.

@Gemba
Copy link
Copy Markdown
Owner

Gemba commented Apr 4, 2025

Ah. Thanks a lot. Now I understand/could reproduce the issue.

To formalize it a bit, thus I can shape the solution: For now let gameListFolder be absolute. Referring to your example you would expect that this config:

[main]
relativePaths="true"
gameListFolder="/path/to/pegasus"
mediaFolder="../media"
inputFolder="../roms"

Will output the gamelist file for pegasus in /path/to/pegasus/ with this content:

collection: Super Nintendo
shortname: snes
launch: /opt/retropie/supplementary/runcommand/runcommand.sh 0 _SYS_ snes "{file.path}"

game: Super Mario World
file: ../roms/Super Mario World.smc
assets.screenshot: ../media/snes/screenshots/Super Mario World.png
assets.boxFront: ../media/snes/covers/Super Mario World.png
assets.marquee: ../media/snes/marquees/Super Mario World.png
assets.wheel: ../media/snes/wheels/Super Mario World.png

And if relativePaths is off/false the asset paths should be in the canonical form of /path/to/pegasus/../media/snes/[...]. For the file accordingly /path/to/pegasus/../roms/Super Mario World.smc

However, if relativePaths is on/true and inputFolder and/or mediaFolder are configured as absolute path, the fallback wlll be the absolute path of these properties if no relative path can be computed in relation to gameListFolder.

Last case: If relativePaths is off/false and inputFolder and/or mediaFolder are configured as absolute path, no relative path computation is tried (the relative folder path is covered two sentences above).

Right?

@cameronhimself
Copy link
Copy Markdown
Author

That all sounds correct to me!

@Gemba
Copy link
Copy Markdown
Owner

Gemba commented Apr 9, 2025

Ok. Leave it with me. I will let you know when it is ready for testing.

@Gemba
Copy link
Copy Markdown
Owner

Gemba commented Apr 14, 2025

It is there for testing @cameronhimself . I did a comprehensive set of tests, but please do also verify if the gamelist / input / media folder settings and command line switches work as expected and discussed before and if you notice any regression.

I also addressed the absolute path for -g and -c while I was on it, they also may be relative paths now.

The mechanics are documented in docs/PATHHANDLING.md.
The flowchart you can find here.

As nothing else has changed (no new configs/settings), you may start the development build from Skyscraper directly where you build the binary, no need to run make install.

@Gemba
Copy link
Copy Markdown
Owner

Gemba commented Apr 27, 2025

Closing this as it is processed with #155.

@cameronhimself feel free to create an issue whenever you should detect a bug (or brain puzzle) regarding this.

@Gemba Gemba closed this Apr 27, 2025
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.

2 participants