tauqua / gog-galaxy-itch.io

An integration connecting itch.io to GOG Galaxy 2.0 using itch's v2 API
GNU General Public License v3.0
221 stars 14 forks source link

Ability to identify potentially unknown games (see #4) #36

Closed nightblade9 closed 3 years ago

nightblade9 commented 3 years ago

Added new flag and code to synchronously poke GOG for game details Added rate limiting to try and prevent getting 401s or API call failures

nightblade9 commented 3 years ago

I need help with this. I've reached the point where it prints out a list, but it always ends up failing at some point, and I don't know how to debug it. Symptoms of failure:

1) Fails after parsing a random number of games - not the same each time (but the order of games is the same). 2) Failure shows up in the GOG UI as "Plugin has crashed" 3) Plugin logs don't show any failures at/around the time of failure

cheater commented 3 years ago

sounds like a parser crashing on unexpected input.

cheater commented 3 years ago

i don't know what GOG does, but i'm going to go ahead and assume that its multitasking works like twisted or gunicorn. doing a sleep(0.5) is going to halt the reactor, making the whole thing crashy. instead of using sleep, sleep by using a callback/future/promise/whatever they call it these days.

nightblade9 commented 3 years ago

Seems like you're right. My Python is very rusty. urllib is synchronous; when I switch it out to using the async HTTP client, and add async to parse_json_into_games and call it with await, the problem goes away.

I think GOG was killing the plugin since it didn't finish initializing in time.

I'll keep testing (I have 1741 Itch games from the BLM bundle) for a bit longer, and add a new commit once everything is solid.

cheater commented 3 years ago

yw, i'd keep the rate limiting in though, who knows what'll happen on other people's computers, and you can't really test for it. you don't want people getting banned from the api and complaining the thing doesn't work.

nightblade9 commented 3 years ago

True, although I have pretty fast internet, it's "naturally" rate-limited for me.

How do I make an async thread sleep? I've never used async in Python before this.

cheater commented 3 years ago

https://stackoverflow.com/questions/56729764/python-3-7-asyncio-sleep-and-time-sleep

cheater commented 3 years ago

Here's the thing, sleep isn't equal to rate limiting. For someone who has slow internet, you don't want to sleep for a fixed amount of time. You want to remember the last time you did a sleep (in this case done using asyncio), calculate how much time should happen between requests, and then sleep the remainder of the time.

nightblade9 commented 3 years ago

Here's the thing, sleep isn't equal to rate limiting. For someone who has slow internet, you don't want to sleep for a fixed amount of time. You want to remember the last time you did a sleep (in this case done using asyncio), calculate how much time should happen between requests, and then sleep the remainder of the time.

True. Actually, you could minimize the sleep time by calculating how long the request took, and sleeping the difference between that and the target value. If your target is 0.5s and the request took 0.3s, you sleep 0.2s.

But honestly, given the extra code to do that, and how it doesn't even need sleep with faster internet - I'd rather go with a simple, clean sleep of a fixed time interval. Doesn't harm anything, really.

cheater commented 3 years ago

This will likely log the same games over and over. I suggest logging the date and time for each line you write to the unkown games log, so it's easier to figure out when to stop reading. I'd use ISO format.

cheater commented 3 years ago

looks great to me otherwise btw

nightblade9 commented 3 years ago

This will likely log the same games over and over.

Correct. However, this only runs when you disconnect and reconnect your integration; it doesn't happen when you sync integrations (I ran a sync and it didn't add anything in my logs0. So, I don't think this is a problem.

looks great to me otherwise btw

Great! Will you be able to merge it in? Or, am I expected to keep/maintain this fork, or maybe just use it to find my own potentially unknown games (which I did now)?

cheater commented 3 years ago

it's a log... there isn't a reason not to add timestamps

nightblade9 commented 3 years ago

Done (and squashed). What's the next step?

cheater commented 3 years ago

wait for @tauqua to merge it in I think

nightblade9 commented 3 years ago

I was looking at this again today. I noticed that although I have 1800 games in Itch (BLM bundle), only around 1300 show up when I add the integration and synch it.

Can anyone else test/verify if they have a similar behaviour?

nightblade9 commented 3 years ago

My bad; I forgot I had replaced my local plugin with the vanilla one, not the one with my changes. So it's not related to my changes.

tauqua commented 3 years ago

Apologies for the way too long time since I came to this. I don't feel comfortable merging this yet because

  1. I have some other features in progress on my dev branch that seem to create merge conflicts with this PR
  2. I think we should clarify the goal of these changes, mainly whether we want to compare against GOG's GamesDB or the IGDB API directly, and
  3. I don't feel comfortable distributing code not directly related to Galaxy's functionality inside a Galaxy plugin

Especially for reason 3, I think this would be better served as a standalone script which compares Itch, GamesDB, and IGDB using three separate API calls. I'm willing to help as I'm able with this, but I don't think this belongs in this plugin.

Thanks so much for your work.

nightblade9 commented 3 years ago

It's too bad I spent so much effort on this and it's going in the trash. I thought I was designing it in the direction you wanted, I guess not.

Lesson learned for me for the future: ask for more feedback, sooner, and wait before moving forward.

tauqua commented 3 years ago

I'm sorry, I could have been clearer earlier, but also I do think that what you've written here can very easily translate to a standalone program, so it doesn't need to go in the trash. Basically if we just take your branch and strip out the Galaxy methods, replacing it by calling your method directly.

cheater commented 3 years ago

I'm not sure why this doesn't belong in the plugin? There are games which the plugin doesn't recognize. We want to know which. This clearly belongs in there.

tauqua commented 3 years ago

The purpose of the plugin is to allow Galaxy to connect with Itch.io. At best, I would consider integrating this as a separate script file distributed with the plugin, that a user can run as they please. My specific objection is to including this in the primary plugin code, there are many reasons for keeping it as simple as possible (security, speed). I also think, as a general usability issue, that it makes more sense to include separately so as not to rely on when Galaxy calls methods, which can be unreliable.

cheater commented 3 years ago

OK, but here's the thing. The plugin is to allow Galaxy to connect with Itch.io. Sometimes, this connection doesn't work - games show up as "unknown". You have no way of properly displaying, in Galaxy's main UI, which games those are. But you should either be displaying this somewhere, or telling the user an error, that a specific game they have on itch is "unknown". You can create a popup (annoying) or a log (less annoying) or a display somewhere else in Galaxy (probably best). As I understand, this code accomplishes one of those. Please correct me if I'm wrong on what the code does?