PeterCat12 / pysmash

A python wrapper to smash.gg Developer API
22 stars 11 forks source link

Not returning all sets #41

Open LuNoX opened 7 years ago

LuNoX commented 7 years ago

Some querys for sets don't return all sets. For example the query for the bracket with id 273020 using bracket_show_set only returns 1 set with id 6460971. It instead should be returning 3 as seen here.

It does the same with the bracket with id 269808.

I have no idea what could be causing this and it was fine about two weeks ago and now it just doesn't work anymore. My suspicion is that the smash.gg adding the search feature messed with some parts of the api.

LuNoX commented 7 years ago

How would I get pysmash to print its stuff so that I can trace it?

LuNoX commented 7 years ago

I found it:

if bracket_set['winnerId'] is None or bracket_set['loserId'] is None: return None, False

This is my issue. Can you add an arg that disables that part of the code? I only need the sets that haven't been played yet.

PeterCat12 commented 7 years ago

Is that tournament currently in progress?

LuNoX commented 7 years ago

I mean it's a test tournament but in theory the matches are in process. It's just that I need the matches that are going to be played, so that I can get the names of the players and notify them. That's what my Discord bot is trying to do.

PeterCat12 commented 7 years ago

The reason that I ask is that SmashGG creates set object for sets that will never complete. For example, if two brackets lead into a finals bracket, and only the top 4 people from each pools bracket moves on. See: https://smash.gg/tournament/hidden-bosses-4-0/events/wii-u-singles/brackets/70445/224997.

A set object is created for Dom and Purin even though those two players never played (because they feed into the finals bracket). Currently I do not think there is a way to distinguish between sets like those (useless/dead sets) and sets that are "in progress".

I will try to look into this however.

PeterCat12 commented 7 years ago

Also, I have noticed problems with my own application since SmashGGs recent update. What specifically was working/returning properly and now is not?

LuNoX commented 7 years ago

So far it's only been those set return methods. Would appreciate it if they got an arg or _all extension or something so that I could still do my thing.

PeterCat12 commented 7 years ago

you used the plural form of "method", which functions specifically would you like that filter on? Also by "working" 2 weeks ago you WERE in fact, getting correct results from these functions previously?

PeterCat12 commented 7 years ago

If you look at this commit made 24 days ago, https://github.com/PeterCat12/pysmash/commit/7ef17c03a0cf05c5ece20851ce07eb560539fbd2 this looks like it would affect what it is you're trying to do. However, if it were working 2 weeks ago (assuming you were using an updated master branch at the time) the error would more likely be due to some change on SmashGG's end and not pysmash (since they updated their code ~2 weeks ago). This weekend I will fully investigate this issue as it is holding up my project.

LuNoX commented 7 years ago

tournaments_show_sets and bracket_show_sets

It seems unlikely that I did because if I did that mean that smash.gg must have had something that prevented it from filtering those particular sets. What I think is more likely is that I used some weird way of copying over the files to the bot so that it would work temporarily. This however seems also unlikely since my forked version clearly has that one line...

So I really don't know why it worked, I just know that it did.

PeterCat12 commented 7 years ago

sorry to bombard you with messages lol. I get a little frantic sometimes. I appreciate your feedback :)

LuNoX commented 7 years ago

It's fine I'm just slow with responding because I'm testing atm as well.

PeterCat12 commented 7 years ago

no worries. Apologies for the problems. I wish that SmashGG's API wasn't the shit show that it is.

LuNoX commented 7 years ago

I mean it is still in development but at least a real doc would be nice...

PeterCat12 commented 7 years ago

I would settle for migration logs lol.

LuNoX commented 7 years ago

Any progress?

PeterCat12 commented 7 years ago

Can you confirm if your still not seeing the sets? It seems that smashGG has fixed whatever changes they made from their last update. Curious to see if this has had any affect.

LuNoX commented 7 years ago

Atm I dont get ANY sets

Edit: nevermind I'm stupid I actually just get the same output as before

PeterCat12 commented 7 years ago

Okay. that makes some sense.I actually did not have time to look into this over the weekend. But I will for sure find time this weekend. Sorry for the delay.

CoinOperatedGadget commented 6 years ago

Any progress on this? I'm actually running into similar problems atm, as my interests are for both tournaments that have ended (for logging reasons) as well as tournaments that are currently underway (for streaming reasons).

I'ma look more into it tonight when I get a chance!

CoinOperatedGadget commented 6 years ago

After taking a look at some samples from the API, I think the unreachable parameter can be used to filter out the sets that Smash.GG uses as placeholders for phases that aren't the final bracket phase. In the pools I looked at it was only set to true for WFs, GFs, LFs, and Loser's Semis for a phase where 4 move on, and WFs, GFs, and LFs in a phase where 3 moved on.

PeterCat12 commented 6 years ago

@gatesnp It's been awhile since I've looked at this issue. If you're confident that you have a solution would you be willing to open a pull request and cover with a unit test?

CoinOperatedGadget commented 6 years ago

@PeterCat12 Can do. Only thing I hesitate about is this will make it so sets that haven't been played are returned (when they haven't been in the past). That could break things for some people if they're assuming only completed sets will be returned as it is right now.

PeterCat12 commented 6 years ago

@gatesnp Ah I am actually one of those people :(

Perhaps we could make a second endpoint that doesn't care whether or not sets have been played or not? Does that make sense to do?

CoinOperatedGadget commented 6 years ago

@PeterCat12 Different end points could work. The issue is different, both are related to tournaments that have yet to complete though. I'd have to look into more, but #46 I believe is just due to some dynamic stuff Smash.gg does while going from stage to stage and has to do with players. This issue is about being able to retrieve sets that have yet to be played as far as I can tell.

Now that I think of it, it might make sense to have three endpoints. One for completed sets (the current one), one for sets that have yet to be played, but are ready to be played (The one I need for my uses), and one for sets that have yet to be played, but are not ready to be played, as in the bracket hasn't reached that point yet (I don't know what use someone would have for this, but I'm sure that someone could have one in the future, and this way there is a way to actually retrieve those sets).

Does that make sense? Or am I off base?

PeterCat12 commented 6 years ago

@gatesnp I think that makes sense. I just want to make sure you've read my above comment regarding "ghost sets". Again, it's been awhile since I've looked into this but I remember that Smashgg makes sets that are never meant to be played (which was the reason the current functionality is the way it is). This occurs when say, pools feed into a top 64 bracket (and when top 64 feeds into top 32, etc).

The reason that I ask is that SmashGG creates set object for sets that will never complete. For example, if >two brackets lead into a finals bracket, and only the top 4 people from each pools bracket moves on. See: >https://smash.gg/tournament/hidden-bosses-4-0/events/wii-u-singles/brackets/70445/224997.

A set object is created for Dom and Purin even though those two players never played (because they feed >into the finals bracket). Currently I do not think there is a way to distinguish between sets like those >(useless/dead sets) and sets that are "in progress".

PeterCat12 commented 6 years ago

Smashgg does tend to silently update their API so my understanding could be outdated.

CoinOperatedGadget commented 6 years ago

No, Smash.gg still does that, but they include a boolean field named "unreachable" that will tell you if it's one of those ghost sets. This could be a recent addition, I'm not sure. That's what I've been using to filter them out.

CoinOperatedGadget commented 6 years ago

@PeterCat12 Also, something to note. I think it was a recent addition, but Smash.gg now has a "Sandbox mode" for tournaments. I was able to make a test tournament that I could set up and put in any state I need, so I should be able to use that for unit tests if I add the endpoints.

PeterCat12 commented 6 years ago

@gatesnp Ah! sorry I missed when you said that before. That is DEFINITELY a "recent" addition. Okay, just so I understand this correctly. Current functionality ONLY returns sets that have been played and treats "unreachable" (let's use the smashgg term) the same as sets that haven't been played.

I think your 3 way split makes sense. Regarding that, I can't remember the name of the current endpoint but perhaps it would be best to change the name of the function to reflect which sets are being returned.

I also am not opposed to keeping it to one endpoint and just passing in some sort of flag to specify which sets to grab (with a default to grab completed sets?).

Good to know about that sandbox mode! Definitely wasn't there I originally made these bindings.

CoinOperatedGadget commented 6 years ago

@PeterCat12 Yeah that's where it is currently from what I can tell.

I kinda like the idea of the single endpoint with a flag (or maybe three separate boolean flags to add the ability to grab any combination of them defaulted to current behavior), but I'm not attached to anything. I'll throw some stuff together either tonight or Sunday night (Have some company visiting this weekend I'll be entertaining) and make a pull request.

Thanks for being so responsive!

PeterCat12 commented 6 years ago

@gatesnp No, thank you for your help!

CoinOperatedGadget commented 6 years ago

@PeterCat12 I ended up being at a tournament Sunday longer than anticipated and wasn't able to do what I said until today. I went through with what I was talking about on https://github.com/gatesnp/pysmash but after creating a larger tournament it looks as though some "ready to be played" matches are still not reported when I have a large tournament with some pools finished and some pools unfinished. Completed and Future matches appear to be working find (although I'm unsure if future matches are all reported, I'll revisit this later).

I created a 100 Person event with 8 pools that lead into a 24 person final bracket for testing. I've set it to statically have 4 of the pools complete, 3 yet to be started and 1 having a single match complete. You can see the tournament here (if the link works, not sure with sandbox mode):

https://smash.gg/tournament/api-test-tournament-1/events/api-test-stage-one-complete/overview

After playing with this for a little bit, it looks like this is the situation: Matches that are ready to be played will not show up if the bracket hasn't had it's first match reported on yet. The only matches being returned are ones for brackets that have had at least one match reported upon. I'm not sure if this is an internal Smash.GG thing where you need to start the bracket, and maybe I'm doing so by reporting on a match before they're initialized or something to do with the pysmash yet. I'll probably end up looking at this tomorrow night.

I just wanted to give an update on where I was since I told you I'd be giving you a pull request on Sunday =D

PeterCat12 commented 6 years ago

@gatesnp Thanks for the update man. I'll try taking a look at this myself tonight. And that sandbox link works :+1:

PeterCat12 commented 6 years ago

@gatesnp Is this still an issue?

CoinOperatedGadget commented 6 years ago

I never made a pull request because I never got to making tests for the changes >.< That said things work (with the exception of what I mentioned in my last comment, but that's a Smash.GG limitation as far as I know). I'll try to get back on this and actually get a pull request together.

PeterCat12 commented 6 years ago

@gatesnp Thanks for the update. Completely forgot about this. If you're lacking time and don't mind me working on your fork, I could add the tests if you open up a Pull Request? I will review code first before doign so but if it looks good I'll go ahead and just commit those tests.

@fickleEfrit

fickleEfrit commented 6 years ago

@gatesnp @PeterCat12 I'm not sure if this comment came before or after I added the check for unreachable, but regardless it's up there now.

fickleEfrit commented 6 years ago

I have made a pull request for this issue.