Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

COMMON: reduce mass add detection output by skipping certain ADGF flags and skipping incomplete file/md5/size matches #3880

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

athrxx
Copy link
Member

@athrxx athrxx commented May 14, 2022

Skip over targets with ADGF_WARNING and ADGF_UNSUPPORTED flags when using "mass add".

The alternative would/could be to just invent a new flag like ADGF_NOMASSADD

For me, this is mostly about fixing bug no. 13282.
https://bugs.scummvm.org/ticket/13282

We sometimes have bogus entries which only have the purpose of presenting the error message (reasons for being unsupported) contained in the extra field of the detection entry.

This PR certainly does not attend to all issues that the "mass add" option currently has, but it does clean it up a bit...

athrxx added 2 commits May 14, 2022
…ags (ADGF_WARNING, ADGF_UNSUPPORTED)

For me, this is mostly about fixing bug no. 13282. We sometimes have bogus entries which only have the purpose of presenting the error message (reasons for being unsupported) contained in the extra field of the detection entry.
Follow-up to the feature that allows skipping certain ADGF flags.

This here now also allows skipping of incomplete file/md5/size matches. It is basically the same behavior as the graylist. For the mass add all files are treated as if they are on the list.
@athrxx
Copy link
Member Author

@athrxx athrxx commented May 15, 2022

I have added another commit that also skips over all results with incomplete file/md5/size matches in mass add mode (basically treating all files as if they're on the AD grayList). I have tested it on the Kyra engine. It now stops detection Donald Duck's Playground and Glassy Ocean in the Eye of the Beholder directories (although I would still like to add the "start.exe" and the "item.dat" to the grayList, since my current changes only concern the mass add mode).

@bluegr
Copy link
Member

@bluegr bluegr commented May 15, 2022

Nice idea! Why do we want to add engines to toggle these flags? The mass add behavior should be the same across all engines unless I'm missing something?

@athrxx
Copy link
Member Author

@athrxx athrxx commented May 15, 2022

Yes, I have used that only for mass detection.

With that other commit I added, maybe it would be better to just have one extra argument like 'bool massDetectionMode' for the detection method...

@bluegr
Copy link
Member

@bluegr bluegr commented May 15, 2022

What I'm saying is: should we expose these as flags to engines as togglable features, or keep the behavior the same across all engines?

There's no reason for engines to modify the behavior of mass add.

@athrxx
Copy link
Member Author

@athrxx athrxx commented May 15, 2022

The flags are set in the detection_tables of the engines for each individual target. They are part of the detection entries. Other than that, I am not sure what kind of exposure you mean.

I have passed the flags in only one place, in the massadd dialog. So that should be the exact same behavior for all engines. But still, not 100% sure if that's what you're asking.

The behavior for all engines: I have really only touched the AdvancedDetector. The few engines that do their own thing (you can easily see from my commits which these are) just ignore the new arguments (except if they also use the AD for some parts of the detection process). Maybe it would make sense to follow up with some handling of the arguments for the rest of the individual detection processes? I don't know. The engine authors who are familiar with their detection code can do that if they want, I guess ..

@athrxx athrxx changed the title COMMON: allow mass add detection to skip certain ADGF flags COMMON: reduce mass add detection output by skipping certain ADGF flags and skipping incomplete file/md5/size matches May 15, 2022
@bluegr
Copy link
Member

@bluegr bluegr commented May 15, 2022

My bad, these are all overridden methods in the engines. My initial though was to put all this logic inside detectGame() without exposing any extra parameter, but then you can't distinguish between a call to add a single game entry from mass add. Sorry for the confusion :)

@athrxx
Copy link
Member Author

@athrxx athrxx commented May 15, 2022

Okay, that's good that this is cleared up :-)

So, while we're making progress here, when I now run mass add in my Kyra directory, it still reports "35 new games, ignored 45 previously added games". So we still have 35 too many ;-)
Maybe I can find and fix that bug, too...
I can even limit it to certain game directories. When I run it in my EOB SegaCD English dir it will return "0 new games, ignored 1 previously added games". But when I use my EOB SegaCD Japanese dir it will return "1 new games, ignored 0 previously added games"... Maybe it's just something with non-English targets. But that's just a guess at this point.

@athrxx
Copy link
Member Author

@athrxx athrxx commented May 19, 2022

I have recently made a fix (committed directly) which further reduces the finds in my Kyra directory.
My impression is that the ones that remain are all valid...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants