Crash when loading gamelists xml

[dszakallas]

ES crashes on startup.

The relevant log line seems to be

Feb 17 13:33:31 lvl2:   Parsing XML file "/home/luigi/.emulationstation/gamelists/scummvm/gamelist.xml"...
terminate called after throwing an instance of 'std::out_of_range'
  what():  map::at
Aborted

Which indicates a problem during loading a game list. Git bisect revealed a113b22ca74228e890b5241201355f86ee43b8c0 as the first commit to contain the issue.

GDB stacktrace

#0  __pthread_kill_implementation (threadid=549617283136, signo=signo@entry=6, no_tid=no_tid@entry=0) at ./nptl/pthread_kill.c:44
#1  0x0000007ff7540a24 in __pthread_kill_internal (signo=6, threadid=<optimized out>) at ./nptl/pthread_kill.c:78
#2  0x0000007ff74fa76c in __GI_raise (sig=sig@entry=6) at ../sysdeps/posix/raise.c:26
#3  0x0000007ff74e74bc in __GI_abort () at ./stdlib/abort.c:79
#4  0x0000007ff77fa578 in __gnu_cxx::__verbose_terminate_handler() () from /lib/aarch64-linux-gnu/libstdc++.so.6
#5  0x0000007ff77f7fcc in ?? () from /lib/aarch64-linux-gnu/libstdc++.so.6
#6  0x0000007ff77f8030 in std::terminate() () from /lib/aarch64-linux-gnu/libstdc++.so.6
#7  0x0000007ff77f8314 in __cxa_throw () from /lib/aarch64-linux-gnu/libstdc++.so.6
#8  0x0000007ff77f0b60 in std::__throw_out_of_range(char const*) () from /lib/aarch64-linux-gnu/libstdc++.so.6
#9  0x0000005555803d1c in std::map<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::less<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::allocator<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > > >::at (this=0x55569260a0, __k="favorite") at /usr/include/c++/12/bits/stl_map.h:560
#10 0x000000555580073c in MetaDataList::get (this=0x5556926098, key="favorite") at /home/luigi/EmulationStation/es-app/src/MetaData.cpp:137
#11 0x000000555581a0cc in FileFilterIndex::getIndexableKey[abi:cxx11](FileData*, FilterIndexType, bool) (this=0x5556950c30, game=0x5556926090, type=FAVORITES_FILTER, getSecondary=false)
    at /home/luigi/EmulationStation/es-app/src/FileFilterIndex.cpp:157
#12 0x000000555581c1d4 in FileFilterIndex::manageFavoritesEntryInIndex (this=0x5556950c30, game=0x5556926090, remove=false) at /home/luigi/EmulationStation/es-app/src/FileFilterIndex.cpp:469
#13 0x000000555581a6c8 in FileFilterIndex::addToIndex (this=0x5556950c30, game=0x5556926090) at /home/luigi/EmulationStation/es-app/src/FileFilterIndex.cpp:191
#14 0x0000005555807f48 in SystemData::indexAllGameFilters (this=0x555694db00, folder=0x55568dcf30) at /home/luigi/EmulationStation/es-app/src/SystemData.cpp:149
#15 0x00000055558076a0 in SystemData::SystemData (this=0x555694db00, name="scummvm", fullName="ScummVM", envData=0x555691c560, themeFolder="scummvm", CollectionSystem=false)
    at /home/luigi/EmulationStation/es-app/src/SystemData.cpp:45
#16 0x0000005555808810 in SystemData::loadSystem (system=...) at /home/luigi/EmulationStation/es-app/src/SystemData.cpp:250
#17 0x000000555580925c in SystemData::loadConfig (window=0x7fffffe890) at /home/luigi/EmulationStation/es-app/src/SystemData.cpp:345
#18 0x00000055557fea98 in loadSystemConfigFile (window=0x7fffffe890, errorString=0x7fffffe7e8) at /home/luigi/EmulationStation/es-app/src/main.cpp:252
#19 0x00000055557fef7c in main (argc=2, argv=0x7ffffff278) at /home/luigi/EmulationStation/es-app/src/main.cpp:362

[pjft]

Interesting - thank you so much for the report and for the thorough debug!

I'm wondering: if you feel comfortable with the code, would you be willing to try to revert the change in this specific line and see if it helps?

https://github.com/RetroPie/EmulationStation/commit/a113b22ca74228e890b5241201355f86ee43b8c0#diff-010d463a0adf9a1ff62b6bfffa379d391c9e396877f53adfe374a92bf18f2f4eR155

Line 155 in Gamelist.cpp changed the way we initialize the game metadata, which is the only thing in that PR that could potentially cause an error in retrieving the "favorite" attribute. It's a weird one, for sure, but if you can test it and let us know that'd be handy as, at least with the current information, I couldn't replicate it here.

If you want to send over your scummvm gamelist.xml as well, it might shed some more light on it - would you potentially have a element on the gamelist?

Tagging @Gemba in case he's not following here.

[dszakallas]

I noticed that the folder MetaDataDecl doesn't have "favorite" attribute among others. Might be triggered by my unconventional folder structure, where each game is actually a folder.

scummvm/
  game.svm/
      game.svm
      ...

This is so I can encapsulate a game in a folder, without that appearing as such in EmulationStation. (lr-scummvm doesnt support zips as far as I know)

I checked the gamelists file. It contained bogus folder entries, that should have been game entries. Not sure if emulationstation or another tool (skyscraper) is the culprit. After replacing folder tags with game tags, the issue is gone.

[pjft]

Well, that's actually even worse to know.

It might be skyscraper (hopefully), but after a few sessions playing those games, please report back on whether the folder elements showed up.

@Gemba it seems that the Filter logic is being tripped up somehow - it does check whether it's a game or not, it seems to be a game but then it was initialized without the "favorite" metadata field which seems to suggest it was initialized as a folder? I was trying to read through the code but it wasn't easy to find where things were going wrong. Happy to have your inputs there.

@dszakallas if you can share a gamelist.xml that has those entries and that crashes (it can just have that entry and nothing more, if it crashes), it'll make it easier for me to replicate and troubleshoot.

Sorry for the trouble, and thank you so much for the help here! Hope you're enjoying some of the new features.

[dszakallas]

I verified that it is skyscraper.

a,

1. I deleted the scummvm gamelist file.
2. I started emulationstation
3. I run a ScummVM game.
4. I quitted emulationstation.
5. Checked the gamelist file. It contained a single game entry with the path to the folder of the game I started.
6. Started emulationstation the second time with success.

b,

1. I deleted the scummvm gamelist file.
2. Regenerated gamelist with skyscraper.
3. Gamelist file contained folder entries for the games.
4. Emulationstation crashed on start.

So it looks like EmulationStation editing works correctly and skyscraper has this issue.

[pjft]

Thank you. If you can share the gamelist that'd be helpful. We must have broken compatibility with it in that change, which means others will be affected.

Also, if you have the full log from ES when it crashes it might give us more hints.

Thank you so much!

[dszakallas]

Here's a minimal gamelist that crashes:

<?xml version="1.0"?>
<gameList>
  <folder>
    <path>/home/luigi/RetroPie/roms/scummvm/3 Skulls of the Toltecs (CD DOS).svm</path>
    <name>3 Skulls of the Toltecs</name>
    <thumbnail />
    <image>/home/luigi/.emulationstation/downloaded_media/scummvm/screenshots/3 Skulls of the Toltecs (CD DOS).png</image>
    <marquee />
    <video />
    <rating />
    <releasedate>19960101T000000</releasedate>
    <developer>Revistronic</developer>
    <publisher>Time Warner Interactive, Inc.</publisher>
    <genre>Adventure</genre>
    <players>1</players>
    <kidgame>true</kidgame>
  </folder>
</gameList>

There's not much else in the logs, it just loads the other gamelists before it successfully.

[dszakallas]

I'm wondering: if you feel comfortable with the code, would you be willing to try to revert the change in this specific line and see if it helps?

https://github.com/RetroPie/EmulationStation/commit/a113b22ca74228e890b5241201355f86ee43b8c0#diff-010d463a0adf9a1ff62b6bfffa379d391c9e396877f53adfe374a92bf18f2f4eR155

Reverting that single line resolves the crash.

However I think this issue with the generator should be reported to skyscraper.

[pjft]

Thank you so much. This is helpful - we'll look into this shortly.

[dszakallas]

Thanks so much for your prompt help!

[pjft]

By all means, you did most of the work. I'll update here when this is fixed. Thanks again.

[cmitu]

However I think this issue with the generator should be reported to skyscraper.

I don't think Skyscraper is at fault here. ES is supposed to support folders as game entries - ScummVM is one case, Daphne is another. The issue is with the addition of 'folder' metadata PR.

[pjft]

Correct, this is on us.

[Gemba]

Thanks for looping me in. I tried to reproduce with the latest ES main/HEAD (3e23bca): I had no success to reproduce it.

Neither setting of <bool name="ParseGamelistOnly" value="true/false" /> crashed ES. However, when set true: An error message shows up in es_log.txt like this:

Feb 17 16:27:11 lvl0:   Error finding/creating FileData for "/home/pi/RetroPie/roms/scummvm/Violet.svm", skipping.

Folder structure:

scummvm/Violet.svm
├── Violet.svm
└── Violet.zblorb

This is the gamelist part of gamelist.xml for ScummVM, this is the one and only entry for Violet:

        <folder>
            <path>/home/pi/RetroPie/roms/scummvm/Violet.svm</path>
                <name>Violet</name>
                <thumbnail />
                <image>./media/screenshots/Violet.png</image>
                <marquee />
                <texture />
                <video />
                <rating>0.56</rating>
                <desc>Violet is one of interactive fiction's most famous examples [snip]</desc>
                <releasedate>20081001T000000</releasedate>
                <developer>Jeremy Freese</developer>
                <publisher>Jeremy Freese</publisher>
                <genre>Adventure, Interactive fiction</genre>
                <players>1</players>
                <lastplayed>20221210T200223</lastplayed>
                <playcount>4</playcount>
                <kidgame>true</kidgame>
                <favorite>true</favorite>
        </folder>

I tried with and without the <kidgame/> and <favorite/> flags but no crash.

I also tried without having a *.svm file in scummvm/Violet.svm/Violet.svm, thus scummvm/Violet.svm/ but no crash either.

Can we have more context from your side @dszakallas to reproduce the issue?

[Gemba]

Reproduceable when <bool name="ParseGamelistOnly" value="false" /> and <bool name="ThreadedLoading" value="false" />

[Gemba]

There must be different paths as the first if clause states it is a game but the metadata claims it is a folder, thus there is no <favorite/>: https://github.com/RetroPie/EmulationStation/blob/3e23bcac754ea570287304ac1f59162d4cd3c127/es-app/src/FileFilterIndex.cpp#L155-L157

The logic a folder on the filesystem is a game inside ES or a folder on the filesystem is a folder in ES does not sit/fit well.

For scummvm it may help to check the extension to force a folder on FS as game type in ES.

IIRC the launcher for Daphne expects not the *.zip but a handle (real file?) with extension daphne in the path element in a gamelist. Thus RetroPie/roms/daphne/roms/yadda_yadda.zip will be RetroPie/roms/daphne/yadda_yadda.daphne in of a gamelist. Thus checking for extension .daphne on a folder may help in this case too.

[pjft]

I'm not thrilled about creating exceptions for specific systems, as that doesn't quite scale. I'd rather figure out why it works differently in the different code paths. It's likely a concurrency issue.

Do we confirm that the execution path when it crashes states it is a game? We should try to see if the metadata has been initialized at all. I suspect it might not have been initialized for some reason. Or alternatively we are changing the type halfway through the execution after it having been initialized. Those are my two thoughts.

[pjft]

Also, to be clear, whether they are a file or a folder in the file system should be irrelevant here.

For ES, the main difference is that a GAME is something that gets launched, whether a FOLDER isn't launched, and contains GAMEs.

[pjft]

Ok, so here's what's happening:

• In this codepath, it first populates the system (SystemData::populateFolder())with the files in the filesystem. In this case, though, all of them are added as GAME. See SystemData.cpp:116 .
• Then, later on, as we're parsing the gamelist (Gamelist::parseGamelist()) in Gamelist.cpp::findOrCreateFile, the logic between line 48 and 55 likely needs to be updated.

            if(found)
                return treeNode;

            if(type == FOLDER)
            {
                LOG(LogWarning) << "gameList: folder doesn't already exist, won't create";
                return NULL;
            }

First, if it's found, but the metadata type is different, we probably need to delete it and create a new one, or somehow update the metadata on the fly? Unsure of the consequences and what's the best option.

Also, unsure about that logic afterwards where it states that if it's a folder we're not creating it as we didn't find it on the file system? Especially if the user is not scanning the filesystem and is parsing the gamelist only, what would happen here?

Just something to check.

Hope this helps.

[Gemba]

After some more analysis, I doubt it needs changes to ES.

Workaround is to have any filesystem folder which is used in the context of a game launcher (i.e. has a valid extension for the system) as <game/> element and not as <folder/> element in a gamelist.xml. I will address it at Skyscraper.

Thanks anyway for the hints and analysis, @pjft .

[pjft]

But:

• The current format had worked so far for users, I assume?
• Not only that, there will be users that will have the old format in their own gamelists, which means that ES will suddenly crash after they update it.

This does need a change here, I believe. Also, per the description, the folder in the gamelist isn't meat to represent a game element, but rather a folder?

I wouldn't want to disregard this yet, unless we do confirm that this scenario wasn't happening as regular use of skyscraper. But I am assuming that, in this scenario for the user, skyscraper was creating a folder item, and a game item inside the folder? @dszakallas can you comment on this hypothesis, or share an actual gamelist after scraping with skyscraper that we can evaluate?

Also, I'm a bit uncomfortable with not knowing exactly why this doesn't crash when ThreadedLoading is enabled. Does it fail to load the game, but it's a silent, non-crash failure, or does it actually run as intended but for some reason doesn't crash? This would be good to know.

[Gemba]

• The current format had worked so far for users, I assume?

The previous (pre PR) gamelist.xml consisted only of <game/> entries. No <folder/> was persisted. So, yes.

At the moment there is only evidence that scummvm is affected and then only when a folder.svm/ is scraped as game and not file.svm or a folder.svm/file.svm combo is scraped.

doesn't crash when ThreadedLoading is enabled

The thread crashes premature and silently I assume, which has the result that the whole ScummVM is not present on the system carousel.

[pjft]

The previous (pre PR) gamelist.xml consisted only of entries. No was persisted. So, yes.

Got it. But even without persisting, since ES crashes on load, we are to assume that the entry used to be loaded as a GAME whereas now it is loaded as a FOLDER, is that it?

The thread crashes premature and silently I assume, which has the result that the whole ScummVM is not present on the system carousel.

I see. Good find, makes sense. That explains why it doesn't show up on my end as well.

I'm just really wanting to avoid having users in that scenario updating ES and having it crash.

I'm wondering, off the top of my head: isn't this just easier to work around by having the FOLDER metadata structure be the equivalent to the GAME metadata structure? Meaning, there's technically no reason for FOLDERS not to be able to be favorites as well - the only thing that would be harder to justify would be the statistics (and maybe it'd cause issues when saving further down the line), but if it'd avoid unexpected crashes, I'd be game, no pun intended. Or we could just make the metadata getter and setter "safe"? Or we could just fix it properly by matching the automatically created GAME entry with the gamelist FOLDER entry and changing it on the fly or removing the old one and recreating it?

I'm open to suggestions here. It'd be good to work this out.

Thanks all, and have a great evening.

[dszakallas]

I think if we consider a game something that is launchable, while a folder as a collection of games, then by that definition, here folder.svm/ should be a game. It looks like ES is consistent with this, as when after deleting the XML and launching a game, ES considered the FS folder as a game (launchable on the UI), and when quitting, recorded the FS folder in the gamelists file as game and not folder (as can be verified with a. https://github.com/RetroPie/EmulationStation/issues/861#issuecomment-1950219950).

[dszakallas]

But I am assuming that, in this scenario for the user, skyscraper was creating a folder item, and a game item inside the folder? @dszakallas can you comment on this hypothesis, or share an actual gamelist after scraping with skyscraper that we can evaluate?

It just creates the folder item. No game item is created by skyscraper.

[pjft]

Correct. But don't you have, inside folder.svm the actual file with the svm extension? I'd have expected you to have both entries on the gamelist - a folder and a game, and you'd launch the game?

Your file structure suggested that:

scummvm/
  game.svm/
      game.svm

If you can share the gamelist after being scraped that'd be helpful.

Edit: hadn't seen your reply, apologies.

[pjft]

Ok, so I tried a few things out on my end to understand what was happening before and:

1) with a folder entry in gamelist.xml, and parsing gamelist only, the folder entry wasn't loaded.

Feb 18 13:18:28 lvl1:   gameList: folder doesn't already exist, won't create
Feb 18 13:18:28 lvl0:   Error finding/creating FileData for "/home/pi/RetroPie/roms/scummvm/Indiana Jones and the Last Crusade (Floppy DOS VGA).svm", skipping.

2) if we're also scanning the hard drive, the entry is loaded and shows on the gamelist. It launches it as if it were a game (meaning, it is loaded as a GAME since it is found in the path and matches the extension), and upon exiting, it is saved as a GAME entry.

I also confirm that, even with a file inside the folder with the extension, Skyscraper will only create a FOLDER entry, and not scan inside the folder with the expected extension.

So, going back to the original intent of: a) at least not having this crash, and b) ideally having this work in a way that is similar or equivalent to before from an end-user perspective,

I suppose @Gemba that we can effectively match any entries we're loading from the gamelist and, if they match the extensions for the system (generically), we can mandatorily load them as GAMEs?

Thoughts on that?

[Gemba]

I just pushed 3.10.1 of Skyscraper out. Should be good, it generates fs-folders with valid extension (=game launcher) as <game/> and no longer as <folder/>. I have not been able to reproduce crashes or odd behaviour in ES.

However, if one of you still gets this message in the debug log of ES, thinks most likely will de-rail, if you are still able to reach this codeline, there must be an exception for Scummvm and no new generic folder may be created. I have not been able to reach that section with scummvm and fs-folders as launchers after regenerating the gamelist.xml with the Skyscraper 3.10.1.

I also have a cosmetic PR for #841 on the shelf. But I will hold it back until this topic is settled.

[pjft]

That's great, thanks.

We just need to make sure that ES doesn't crash when loading old Gamelists, as users won't necessarily know that the issue is related to the Gamelist they have.

We should strengthen the logic when loading folders from the Gamelist and they match a Game entity from the file system.

[pjft]

@Gemba can you check whether this breaks the Folder persistence logic in any way? This makes ES behave like it used to.

[Gemba]

@pjft I found another cornercase in this cornercase:

Setups like game.svm/game.svm fail as game.svm/ is treated as GAME an thus can not have children. The assertion fails: (https://github.com/RetroPie/EmulationStation/blob/bb6d8e9e47644e583beda533dfea80bce5f70224/es-app/src/FileData.cpp#L219).

I have a patch locally but need to fix my branch to make it a clean PR. This would be my fix:

diff --git a/es-app/src/Gamelist.cpp b/es-app/src/Gamelist.cpp
index 66a92d9..3fd6562 100644
--- a/es-app/src/Gamelist.cpp
+++ b/es-app/src/Gamelist.cpp
@@ -56,8 +56,12 @@ FileData* findOrCreateFile(SystemData* system, const std::string& path, FileType

                        FileData* file = new FileData(type, path, system->getSystemEnvData(), system);

-                       // skipping arcade assets from gamelist
-                       if(!file->isArcadeAsset())
+                       // skipping arcade assets from gamelist and add only to filesystem
+                       // (fs) folders, i.e. entriess in gamelist with <folder/> and not to
+                       // fs-folders which are marked as <game/> in gamelist. NB:
+                       // treeNode's type (=parent) is determined by the element in the
+                       // gamelist and not by the fs-type.
+                       if(!file->isArcadeAsset() && treeNode->getType() == FOLDER)
                        {
                                treeNode->addChild(file);
                        }
@@ -74,7 +78,14 @@ FileData* findOrCreateFile(SystemData* system, const std::string& path, FileType
                                LOG(LogWarning) << "gameList: folder " << absFolder << " absent on fs, no FileData object created. Do remove leftover in gamelist.xml to remediate this warning.";
                                return NULL;
                        }
-
+                       // discard constellations like scummvm/game.svm/game.svm as
+                       // scummvm/game.svm/ is a GAME and not a FOLDER
+                       if (treeNode->getType() == GAME)
+                       {
+                               std::string absFolder = Utils::FileSystem::getAbsolutePath(pathSegment, systemPath);
+                               LOG(LogWarning) << "gameList: trying to add game '" << absFolder << "' to a parent <game/> entry is invalid, no FileData object created. Do remove nested <game/> in gamelist.xml to remediate this warning.";
+                               return NULL;
+                       }
                        // create folder filedata object
                        std::string absPath = Utils::FileSystem::resolveRelativePath(treeNode->getPath() + "/" + pathSegment, systemPath, false, true);
                        FileData* folder = new FileData(FOLDER, absPath, system->getSystemEnvData(), system);

[pjft]

I trust you there, but from my tests, Skyscraper wouldn't have generated that structure - it would have stopped on the Folder entry, and I don't think ES would have gotten there as well.

Can you confirm that that was the case and, if so, would that scenario effectively materialize?

Thanks.

[Gemba]

I tried it with this config.ini in Skyscraper.

[scummvm]
subdirs="true"
includeFiles="*.svm"

And this sample structure:

scummvm/Violet.svm/
├── Violet.svm
└── Violet.zblorb

I can zap those surplus entries (any *.svm file below scummvm/Violet.svm/) in the generated gamelist.xml in Skyscraper but idk how other Scrapers handle that matter.

[pjft]

Got it. Happy to include your patch, but a few questions regarding your first change.The goal of that first code is to skip MAME BIOS and such. With the current condition

if(!file->isArcadeAsset() && treeNode->getType() == FOLDER)
{
        treeNode->addChild(file);
}

We'll now only add things if it's a folder. So what's the equivalent logic for adding GAMEs?

This code checks for matches for entities (well, files) in the file system, and corresponding entries in the gamelist.

From my understanding of reading the code, that condition should run if there isn't a corresponding entry loaded from the filesystem (for instance, if you have enabled the "only parse gamelists" option, if will never be found), so it needs that code to add the FileData. If it isn't found, treeNode is, effectively, the root folder.

I might be wrong, but let's make sure this works well across all the different combinations.

[Gemba]

I have put up a clean PR. Will comment on your questions next.

[Gemba]

From my understanding of reading the code, that condition should run if there isn't a corresponding entry loaded from the filesystem (for instance, if you have enabled the "only parse gamelists" option, if will never be found), so it needs that code to add the FileData. If it isn't found, treeNode is, effectively, the root folder.

Valid point. I understand it logic-wise, do you have a constellation at hand, by chance?

Anyhow, eventually your highlighted if statement abovve should also reflect the sunny day approach (=regular case), which it currently does not. i.e. the mame-libretro/game.zip does not get added to the FileData tree. Let me think of it how to address this.

[pjft]

Hm. It should add it, though? The condition of isArcadeAsset only checks whether the file is in the BIOS or DEVICE files list, and the platform is ARCADE or NEOGEO. mame-libretro/neogeo.zip shouldn't be added (it's a BIOS file), but mame-libretro/mercs.zip should. I might be misreading your comment, though, so please let me know if that's the case.

I think the main scenarios we need to look out for are the "Parse Gamelists Only" being enabled or not.

The sequence is:

• scan filesystem
• parse gamelist data

If you don't have that option enabled, if something isn't found in the filesystem it shouldn't be added to the system. But that's already addressed in line 133:

if(!trustGamelist && !Utils::FileSystem::exists(path))
{
LOG(LogWarning) << "File \"" << path << "\" does not exist! Ignoring.";
continue;
}

so, when you get to the code we're working with, the only possibility is that the file does exist in the filesystem, and we're trying to find if a corresponding FileData object exists (if we scanned the filesystem and created it already). If we haven't created it yet, it can be because we didn't scan the filesystem (ParseGamelistsOnly = true), because if we only get to the code you're adjusting if there actually exists something in the filesystem for that path.

So we need to create a new FileData to show that entity (game? folder?) in ES, normally, unless they fall under one of the existing exceptions.

[Gemba]

Ah, got it. The files directly under a system-folder e.g. mame-libretro/game.zip are covered here.

Thus the change I suggest only applies to any subfolder of a system-folder. Nothing to consider then.

EDIT: I was referring to https://github.com/RetroPie/EmulationStation/issues/861#issuecomment-1952318483, and your question

We'll now only add things if it's a folder. So what's the equivalent logic for adding GAMEs?

A: Those will be covered with these lines https://github.com/RetroPie/EmulationStation/blob/bb6d8e9e47644e583beda533dfea80bce5f70224/es-app/src/Gamelist.cpp#L36-L42

[pjft]

Not sure I follow your comment, but to be clear, what I was saying earlier was that if the user has ParseGamelistsOnly set to true, the code here will return an empty map, and as such found will always be false for games in the main folder.

Unsure if that was what you were referring to, but just making sure we were talking about the same thing :)

[Gemba]

I went back to 6e47f19510b05175df5a26d547acb108987ea904 to vet if your sketched behaviour has been changed since the commits then, i.e. I tested this below with the stated commit and also on main/HEAD:

1. I have set ParseGamelistOnly=true
2. I have picked two random games from mame and put one in a subfolder without extension and also adjusted the <path/> for that <game/> in the gamelist.xml, I did not add a <folder/> element. Let these games and directories be asterix.zip and zax/zaxxon.zip.
3. While it is true that the found flag is initial false after trying to find it in the children map, both games get added at the first treeNode->addChild(file); line here in Gamelist.cpp.

You may check on your side by adding something like

if (file->getFileName() == "zaxxon.zip" || file->getFileName() == "asterix.zip") {
    LOG(LogDebug) << "adding to parent: " << file->getFullPath();
}
treeNode->addChild(file); // this line is already there

before the first treeNode->addChild(file); line in Gamelist.cpp

Adding a && treeNode->getType() == FOLDER to that clause as suggested in #863 does not alter this behaviour as the system-root is always a filesystem directory (f.i. mame-libretro/).

Did I left a question of you pending?

[pjft]

Thanks for checking. All good then - is the other PR ready?

[Gemba]

Thank you too, and yes #863 is ready. Maybe it is advisable to have the merged version checked by the OP.

[pjft]

Good call. @dszakallas would you be able to test the changes in #863 on top of the current emulationstation-dev code and confirm it's all fixed?

Sorry for the trouble, and thanks for your help.

[dszakallas]

I tested it with unchanged skyscraper configuration (i.e which generates folder entries, and no game entries for ScummVM) and #863 on top of latest master works for me. Thanks!

[pjft]

Thank you both. It's merged now!