Skyscraper now officially part of RetroPie, please test

Gunner14

NO hard feelings mate,sorry to have bothered you.How do i delete tbe account on here?

muldjord

@mitu, User request that I think makes sense: When choosing which platforms to start gathering data for, the GUI has the "OK" button. This will start the process immediately which isn't clear. Maybe we should change the "OK" to "START" or something similar (or maybe even add a confirmation dialog with "This will start the gathering process, continue?". Same could be for the game list generation process). In this particular case the user wasn't ready for the process to start, he needed to configure more before doing so. He thought it would just save the platform selection and he'd have to initiate it somewhere else.

EDIT: Even better, we should consider splitting it up, so there's a menu to select which platforms ("Select platforms"), and an extra menu items simply saying "Start resource gathering". We should then remove the "Gather for all" option. I still feel like that option is confusing.
Same for game list generation ("Select platforms" and "Generate game list(s)").

I also feel like we should move the options menu above the select and start menu items. It seems users choose from top and work their way down. So they miss the options before starting the processes.

Let me know what you think of these ideas.

mitu

@muldjord Yes, I've seen that - still pondering about adding it.
The thing is that any other screen in the RetroPie-Setup interface (the dialog based UI) is using this convention (OK/Cancel) and I think users are familiar with it (probably that's why nobody suggested this so far). Plus, the settings/options are clearly marked as such in the UI (enabled/disable, source name, etc.) and for someone that uses the script more than once it's not a problem.
On the other hand there's no doubt it makes sense and since the button is on the 2nd page - accompanied with some choices - it would be probably more obvious to have it as 'Start (the) Gathering' to make the user aware on what the next action is.

Bike shedding at its best :).

muldjord

@mitu Read above, I've pondered a bit further.

mitu

EDIT: Even better, we should consider splitting it up, so there's a menu to select which platforms ("Select platforms"), and an extra menu items simply saying "Start resource gathering".

I feel there's too many steps just to start scraping.

We should then remove the "Gather for all" option. I still feel like that option is confusing.

In 3.x, yes, it's probably something that doesn't fit the process anymore. I think we talked about it before the 3.x version and we said we'll remove it - I do intend to remove it when adding a few other 3.x tweaks (vacuum being the first).

Same for game list generation ("Select platforms" and "Generate game list(s)").

Same comment as above - seems too complicated, bu I can surely give it a try to see how it would look, it might be a bit of a hack to pass down values from the sub-menu to the main UI.
An idea I had for this would be to keep track of what was selected - and gathered - in the menu above and during generation to pass it along, without any need to select the system - mainly to also remind the user that the gamelist generation needs to be run if gathering was previously run.

I also feel like we should move the options menu above the select and start menu items. It seems users choose from top and work their way down. So they miss the options before starting the processes.

That, I don't agree - we already have good defaults in place and I think actions should be 'front and center'.

muldjord

@mitu said in Skyscraper now officially part of RetroPie, please test:

Same comment as above - seems too complicated, bu I can surely give it a try to see how it would look, it might be a bit of a hack to pass down values from the sub-menu to the main UI.

It is a step more, I see that. But it also clarifies the process a bit I think (set it up, then run it), and solves the problem of users accidentally starting the process (that they can't stop with a controller, right?), when they just thought they were selecting the platforms where they would expect to just go back to the menu after having done so.

Either way, removing the "Gather for all" will be a good thing.

Adding the new purge options would also be great, especially --cache vacuum and in time maybe also the --cache purge:all, --cache purge:t=<TYPE>,m=<MODULE> where you could then select which type and / or from which module (they can both be left out).

mitu

@muldjord said in Skyscraper now officially part of RetroPie, please test:

It is a step more, I see that. But it also clarifies the process a bit I think (set it up, then run it), and solves the problem of users accidentally starting the process (that they can't stop with a controller, right?)..

Actually, since running the scraper involves exiting Emulationstation and then starting the RetroPie-Setup script, this means using a keyboard (or a SSH section).
However, some of them might not be aware they can stop the process with Ctrl+C (being unfamiliar with the command line). I think we can show them this info in a confirmation dialog before the start of the process.

muldjord

@mitu said in Skyscraper now officially part of RetroPie, please test:

Actually, since running the scraper involves exiting Emulationstation and then starting the RetroPie-Setup script, this means using a keyboard (or a SSH section).
However, some of them might not be aware they can stop the process with Ctrl+C (being unfamiliar with the command line). I think we can show them this info in a confirmation dialog before the start of the process.

Good point, I forgot about that.

slutsteg

Can you do so that you do not have to quit emulationstation when you need to download pictures or auto quit and relaunch emulationstation?

muldjord

@slutsteg said in Skyscraper now officially part of RetroPie, please test:

Can you do so that you do not have to quit emulationstation when you need to download pictures or auto quit and relaunch emulationstation?

Nope, can't be done. The reason being that ES writes the in-memory game list back to disc when it quits or you restart the system. And when it does, it will overwrite the game list created by any external scraper.

You could run Skyscraper from command line over SSH and gather data while ES is running, that won't be a problem though. Just remember that if you start generating game lists afterwards, they will disappear when ES is restarted or closed down. I tend to forget this myself from time to time and wondering where the hell the new entries I just created had gone. So yeah, even after all this time, I still forget sometimes.

mitu

@muldjord I've made some modifications to the scriptmodule, incorporating some of the feedback from the last discussion.

I removed the 'Gather for all platforms' entry from the menu, which should be now simpler. The gather action should now be similar to the gamelist generation.

I added a warning when starting scraping

Added the vacuum option under the Cache options and commands and switched the purge command from a straight delete to the corresponding --cache purge:all command.
Other small changes in the verbiage and included files.

I tried to test with having a single Select platforms option (for both gathering and generation), but it turned a bit complicated to pass on the parameters and frankly a bit cumbersone - you have to enter a menu, select something, go back, select another entry, then confirm you want to scrape, etc.

muldjord

@mitu This looks good, great work. It's clearer without the "Gather for all" option and the prompt clears up any confusion about how to stop the process, so thumbs up for that aswell.
Using the now included purge options is definitely a great addition too.

mitu

@muldjord Eh, it's mostly incremental, it's not that much.

I had one idea for Skyscraper - the ES built in scraper has a way to modify the search string when querying the scraping source (the Input button in the image below)

One option when modifying the search string is to use the syntax ID:number and the scraper would perform a search-by-id on the site.

I thought you could add this to the --query syntax:

--query id=<gameID>

and query by the ID. Of course, it will depend on the scraper source, but I think it's easier to enter than - for instance - the MD5/CRC when searching on ScreenScraper.

muldjord

@mitu This is already possible for the screenscraper source I believe - but it's probably called gameid or so. For the file name search based sources it is not though.

EDIT: No, that doesn't seem to work. I wonder why.

mitu

@muldjord I thought you might have implemented this and checked the docs (clihelp.md), but I didn't see it mentioned (and it doesn't appear in the --help output also).
I reported a bug in the V2 WebApi last week (the same forum thread on the ScreenScraper site) where searching by gameid didn't work just with the ID, so it might not have worked before - should work now, I tested it in the ES scraper. I think TGDB should also work with the ID - I know it worked before and I've seen it in the new added to the new ES TGDB scraper version (see here).

muldjord

@mitu, are you aware of this? https://github.com/muldjord/skyscraper/issues/134

I've tested the purge command and it works just fine. It shouldn't even be looking for artwork.xml if the command line is correct for it. I don't use the script myself so I can't test.

mitu

@muldjord No, but I'll take a look.

EDIT: I think I know why it fails - I'll submit a fix tomorrow. The old purge used simple rm commands, the new method uses the Skyscraper's command, but I forgot to make it run as the normal user. Since the RetroPie setup runs as root, the error is probably because that user doesn't have Skyscraper set-up properly.

muldjord

@mitu said in Skyscraper now officially part of RetroPie, please test:

EDIT: I think I know why it fails - I'll submit a fix tomorrow. The old purge used simple rm commands, the new method uses the Skyscraper's command, but I forgot to make it run as the normal user. Since the RetroPie setup runs as root, the error is probably because that user doesn't have Skyscraper set-up properly.

Sounds plausible. Thanks for looking into it.

2fst4u

@mitu hi there, submitter of that issue here. Thanks for looking into it and good to hear that it shouldn't be too difficult (relatively) to sort out.

In the meantime, is there anything manual that I can try to make it work or would it be best to just wait for a fix?

mitu

@2fst4u Hello and thank you for the bug report.
I've submitted a fix for the problem you reported (https://github.com/RetroPie/RetroPie-Setup/pull/2658) - I think you should wait to be added to the RetroPie-Setup script and then just update to get the fix.