Skip mame bios/devices #432

raelgc · 2018-05-10T01:31:42Z

New attempt as now mame files were moved to external files.

raelgc · 2018-05-10T11:35:13Z

@joolswills New attempt after the my old PR #329, when mame stuff was hardcoded.

joolswills · 2018-05-10T19:01:47Z

I'm happy to merge this if others are.

tomaz82 · 2018-05-10T19:44:44Z

I'm not, first off it has changes that is pure code style changes, to one that is not matching how ES style is.
It has missing newline at the end of both xml files, the fast manual search function that is used for the mamenames is not used for the asset, it uses the slower std::find. And that's just the ones I found after a quick 20 second look.

I would advice against a merge until all these issues are adressed.

joolswills · 2018-05-10T19:46:31Z

Sorry. I missed the styling issues - I agree with your comments @tomaz82

joolswills · 2018-05-10T19:46:50Z

es-app/src/SystemData.cpp

@@ -112,7 +112,8 @@ void SystemData::populateFolder(FileData* folder)
 				continue;

 			FileData* newGame = new FileData(GAME, filePath, mEnvData, this);
-			folder->addChild(newGame);
+			if(!newGame->isArcadeAsset())
+			  folder->addChild(newGame);


joolswills · 2018-05-10T19:47:09Z

es-core/src/MameNames.cpp

 	std::string xmlpath = ResourceManager::getInstance()->getResourcePath(":/mamenames.xml");
-
 	if(!Utils::FileSystem::exists(xmlpath))


unrelated whitespace changes.

joolswills · 2018-05-10T19:48:37Z

resources/mamedevices.xml

+<device>dmv_k235</device>
+<device>mc1502_rom</device>
+<device>ql_pcmlqdi</device>
+<device>gfxultrp</device>


no newlines at eof in xmls

joolswills · 2018-05-10T19:50:05Z

es-core/src/MameNames.h

+	static void              deinit     ();
+	static MameNames*        getInstance();
+	const bool               isAsset    (const std::string& _mameName);
+	std::string              getRealName(const std::string& _mameName);


no need for styling changes here. if you think the indent is wrong, it can be done as a separate later commit.

raelgc · 2018-05-10T20:07:00Z

Oh my, for some reason atom gone crazy with tabs (it should replace by 2 spaces). Let me fix all pointed issues.

joolswills · 2018-05-10T20:13:09Z

Using tabs is correct for the ES sources.

raelgc · 2018-05-10T20:32:35Z

While working in the fix, I can see the existing mamenames.xml has no newline at eof. Should I add to it too?

raelgc · 2018-05-10T20:44:38Z

@tomaz82 Why not, then, use the standard std::binary_search instead of a manual search?

raelgc · 2018-05-10T21:06:34Z

Ok, I've made all requested fixes using gedit as editor.

tomaz82 · 2018-05-10T21:31:26Z

@raelgc because that "standard" std::binary_search still requires algorithm included ( which most likely will include even more headers ) which is pointless when we have a fast search algorithm there already.

raelgc · 2018-05-10T22:00:14Z

But we already have algorithm included in other files. Why should I write my own binary search when the standard c++ has a good one, in a library already included in the system?

tomaz82 · 2018-05-11T08:35:37Z

I never said you should write your own, I said you should use the one VERY similar to the one we have in getRealName, it's your way of thinking about code that had ES take 50+ minutes to compile on the RPi3 before I optimized it down to less than 10 minutes.

raelgc · 2018-05-11T10:36:05Z

Interesting @tomaz82, I was not aware of the compilation time. Thanks for explaining. It's a really fare argument!

In defense of the algorithm, I just used because it was used in other files. But following your advice, I'll replace by a manual a binary search. For this, I'll need to sort the Mame assets vector (as I'm combining the bios xml and the devices xml). To avoid std::sort, should I merge them in just one file?

Just to share: for someone using Atom and the breaking styling: I gone to Preferences > Editor and disable Atomic Soft Tabs (it makes the cursor jumps throw white-spaces like tabs, so you'll think you have tabs) and Soft Tabs (it tries to guess when use tabs vs spacing, but for me it was replacing the tabs for white-spaces).

tomaz82 · 2018-05-11T12:39:33Z

@raelgc I understand, but what other files do, doesn't affect the compiletime of this file.
Looking at it more closely, I'd keep the 2 xml's entirely separated, even when stored as vectors, and make 2 functions ( isDevice and isBios ) instead of the isAsset,

raelgc · 2018-05-11T13:05:43Z

@tomaz82 Sure, I'll work on this.

raelgc · 2018-05-11T18:44:57Z

@tomaz82 Just pushed the separated list for bios and device! :)

raelgc · 2018-06-05T18:25:19Z

@joolswills and @tomaz82: is it good to go now?

raelgc · 2018-06-25T10:40:12Z

@pjft Do you mind to test and give any feedback?

Expected result is to not see mame devices in the gamelist, by example, neogeo.zip in mame/neogeo/arcade gamelist.

pjft · 2018-06-26T08:51:15Z

Hi. I don't have much time this week, apologies. I'll give it a go when I have the chance, but I suspect the main feedback here/green light would have to come from @tomaz82 as he's the one who had specific concerns about the code and libraries being used. I see you're still including , which I believe was one of the concerns @tomaz82 raised, unless I missed something else here.

tomaz82 · 2018-06-26T10:36:23Z

I'm still waiting for the requested changes

raelgc · 2018-06-26T10:58:51Z

@tomaz82 Just removed algorithm library. I've separated the 2 lists like you requested in a previous commit.

pjft · 2018-06-29T16:19:34Z

So, I had a few moments today and compiled this and tested.

Still, the BIOS files still show up on my arcade set. Am I meant to enable one option? At least neogeo.zip and pgm.zip show up.

pjft · 2018-07-17T17:45:48Z

Well, see if it could be applied in a single place then - I'm not especially fond of the idea of duplicating logic unless really necessary.

Thanks.

raelgc · 2018-07-20T23:32:27Z

@pjft I just pushed a change to prevent arcade assets get read from gamelist.xml. When you have some free time, please, can you test again?

jrassa · 2018-07-25T02:27:27Z

es-app/src/Gamelist.cpp

@@ -128,11 +133,11 @@ void parseGamelist(SystemData* system)
 			//load the metadata
 			std::string defaultName = file->metadata.get("name");
 			file->metadata = MetaDataList::createFromXML(GAME_METADATA, fileNode, relativeTo);
-
+			


unnecessary whitespace added

jrassa · 2018-07-25T02:27:34Z

es-app/src/Gamelist.cpp

 			//make sure name gets set if one didn't exist
 			if(file->metadata.get("name").empty())
 				file->metadata.set("name", defaultName);
-
+			


unnecessary whitespace added

jrassa · 2018-07-25T02:27:43Z

es-app/src/SystemData.cpp

+			if(!newGame->isArcadeAsset())
+			{
+				folder->addChild(newGame);
+				isGame = true;				


trailing tabs

raelgc · 2018-07-25T10:47:57Z

@jrassa Just made the requested changes.

pjft · 2018-07-27T17:32:46Z

Hi!

I'm so sorry for the delay, and also for the update. For some reason, they are still showing.

I'm running the binary from commit

baa9cff

from 2 days ago :/

The resources from the gamelists are still showing somehow - at least neogeo.zip and pgm.zip .

raelgc · 2018-07-30T16:56:10Z

@pjft Sounds like I'm still missing some other entry points.

Any chance your ES is configured to something different than the default? I mean, a filter applied from our tests on the improved Kids mode, something like that?

pjft · 2018-07-30T19:00:20Z

I don't expect that to be the case, but I'll look into it in the coming days and get back to you.

raelgc · 2018-07-30T19:08:43Z

@pjft Just to share: I tested both cases (mame assets outside the gamelist), and later manually added assets in the gamelist.xml. Both with a fresh retropie install (and they worked) in 2 different machines.

Just wondering which is the difference between our setups. For this reason I'm mentioning the filters. But of course, can be other features.

pjft · 2018-07-30T20:24:09Z

es-app/src/FileData.cpp

+const bool FileData::isArcadeAsset()
+{
+	const std::string stem = Utils::FileSystem::getStem(mPath);
+	return ( 


Trailing space here, it seems?

pjft · 2018-07-30T20:32:35Z

es-app/src/Gamelist.cpp

-			treeNode->addChild(file);
+
+			// skipping arcade assets from gamelist
+			if(!treeNode->isArcadeAsset())


I can confirm that in my scenario, it does not go through here.

es-app/src/SystemData.cpp

pjft · 2018-07-30T21:02:47Z

es-app/src/Gamelist.cpp

-			treeNode->addChild(file);
+
+			// skipping arcade assets from gamelist
+			if(!treeNode->isArcadeAsset())


Here's your error. treeNode will be the parent folder in this case. You probably want to run this check against "file", which is the newly-created file.

That being said, I'm not fully comfortable with the idea of even creating a FileData object if we're going to discard it - I'd rather only create it if we're going to use it, otherwise make sure to return NULL - and see if we don't crash.

@pjft First: thank you for your time and patience.

I just pushed the requested changes but not yet the NULL return (before double check with you).

If I return NULL, it'll not crash, but it'll produce this output:

lvl0: Error finding/creating FileData for "/home/rael/RetroPie/roms/neogeo/neogeo.zip", skipping.`

Returning the FileData, no error output.

How should I proceed?

I'd leave a definitive answer for others to chime in on, but in my perspective I'm not wholly bothered by that warning. The alternative is to create FileData objects that are dangling without a parent, take up memory, and never get explicitly removed. I'd imagine there'd be some edge case where that'd come and bite us back.

But if anyone else has any recommendation or preference, I'd happily take that.

raelgc · 2018-09-10T23:35:06Z

@pjft Sorry for the long time away from this. But how about now? I'm avoiding the error message and avoiding create any additional metadata with the FileData when it's an arcade asset.

pjft · 2018-09-14T19:50:50Z

Seems to work well, from my brief testing.

raelgc · 2018-09-18T15:24:02Z

@tomaz82 and @joolswills I think we're good to go now (i.e., I've addressed all issues pointed by you guys, thanks for that).

raelgc · 2018-10-30T13:22:31Z

@joolswills: in the last weeks, I just got playing with settings and thinking about you've said about performance and focus on Raspberry devices. And probably today the only "annoying" BIOS we have displayed in the gamelist is usually neogeo.zip, which can be moved to the ~/Retropie/BIOS folder (at least for lr-fbalpha).

So, in a second moment, I'm no more inclined to include all this overhead that my PR will produce just to handle few files we can easily get hidden.

What do you think?

dankcushions · 2018-10-30T13:57:57Z

i still think the PR is a good idea. all other ways of hiding BIOS files need manual intervention from the user.

neogeo.zip is needed in the roms folder for MAME cores, and it's not the only BIOS file needed in MAME - 2003 also needs skns, pgm, and taitofx1. later versions of MAME will have need even more BIOS files.

fbalpha has neogeo.zip (which can be hidden via the BIOS folder, but that requires manual intervention by the user, and knowledge of that niche behaviour), but it also requires isgsm, nmk004, pgm, skns, and ym2608.

raelgc · 2018-11-04T17:29:23Z

Well, in any case, I just squashed the commits.

joolswills · 2018-11-14T23:43:29Z

I'm going to merge this unless anyone has an issue

joolswills · 2018-11-19T05:36:44Z

Thanks for your work on this.

Skip mame bios/devices

joolswills reviewed May 10, 2018

View reviewed changes

raelgc force-pushed the master branch 3 times, most recently from 258aeee to 93956ab Compare May 10, 2018 21:03

raelgc force-pushed the master branch from 93956ab to c322e80 Compare May 10, 2018 21:08

jrassa reviewed Jul 25, 2018

View reviewed changes

pjft reviewed Jul 30, 2018

View reviewed changes

es-app/src/SystemData.cpp Show resolved Hide resolved

pjft reviewed Jul 30, 2018

View reviewed changes

raelgc added a commit to raelgc/EmulationStation that referenced this pull request Sep 10, 2018

Avoiding the error log but avoid keep adding metadata (RetroPie#432)

be98c9a

raelgc added a commit to raelgc/EmulationStation that referenced this pull request Sep 18, 2018

Avoiding the error log but avoid keep adding metadata (RetroPie#432)

bace47f

raelgc mentioned this pull request Sep 18, 2018

Forgot to make the ForceDisableFilters persists after restart (#431) #480

Closed

Skip mame bios/devices

065fd8e

raelgc force-pushed the master branch from be98c9a to 065fd8e Compare November 4, 2018 17:27

joolswills merged commit bf02819 into RetroPie:master Nov 19, 2018

5schatten mentioned this pull request Dec 23, 2018

Skip mame bios/devices mmatyas/pegasus-frontend#363

Closed

jrassa mentioned this pull request Aug 5, 2019

Automatically hide various BIOS/non-game files #89

Closed

Yaoh pushed a commit to Yaoh/EmulationStation that referenced this pull request Jul 13, 2020

Merge pull request RetroPie#432 from raelgc/master

dd522e1

Skip mame bios/devices

pjft mentioned this pull request Aug 12, 2021

Ignore filenames when loading systems #761

Open

		std::string xmlpath = ResourceManager::getInstance()->getResourcePath(":/mamenames.xml");

		if(!Utils::FileSystem::exists(xmlpath))

Skip mame bios/devices #432

Skip mame bios/devices #432

Conversation

raelgc commented May 10, 2018

raelgc commented May 10, 2018

joolswills commented May 10, 2018

tomaz82 commented May 10, 2018

joolswills commented May 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raelgc commented May 10, 2018

joolswills commented May 10, 2018

raelgc commented May 10, 2018

raelgc commented May 10, 2018

raelgc commented May 10, 2018

tomaz82 commented May 10, 2018

raelgc commented May 10, 2018

tomaz82 commented May 11, 2018

raelgc commented May 11, 2018 • edited

tomaz82 commented May 11, 2018

raelgc commented May 11, 2018

raelgc commented May 11, 2018

raelgc commented Jun 5, 2018

raelgc commented Jun 25, 2018

pjft commented Jun 26, 2018

tomaz82 commented Jun 26, 2018

raelgc commented Jun 26, 2018

pjft commented Jun 29, 2018

pjft commented Jul 17, 2018

raelgc commented Jul 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raelgc commented Jul 25, 2018

pjft commented Jul 27, 2018

raelgc commented Jul 30, 2018

pjft commented Jul 30, 2018

raelgc commented Jul 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raelgc commented Sep 10, 2018

pjft commented Sep 14, 2018

raelgc commented Sep 18, 2018

raelgc commented Oct 30, 2018

dankcushions commented Oct 30, 2018

raelgc commented Nov 4, 2018

joolswills commented Nov 14, 2018

joolswills commented Nov 19, 2018

raelgc commented May 11, 2018 •

edited