jaeha-choi / DFF

A program that fetches data from op.gg to set runes, item pages, and spells automatically.
BSD 3-Clause "New" or "Revised" License
1 stars 0 forks source link

Champs with no statistics (RIP) crashes the app #3

Closed ThePridestalker closed 2 years ago

ThePridestalker commented 2 years ago

Champs like shyvana or skarner currently, showing as RIP in https://www.op.gg/champion/statistics, have builds but no role, causing the app to crash

jaeha-choi commented 2 years ago

Seems like this issue is caused by a parsing error. Current implementation crashes the application when the error is encountered (very bad design, I agree).

I'm already working on re-writing the code to prevent the app from crashing upon error. However, that solution isn't necessarily helpful in this case, as DFF wouldn't be able to set up runes/spells/items even when the app does not crash.

I'll need to think about a better way to provide information for champions that aren't used too often. At the moment, I'm thinking of allowing users to manually select the role themselves and warn them about the sample size. Do you have any better ideas?

ThePridestalker commented 2 years ago

Can you make the DFF window get on top of everything in case of errors or the lack of builds?

I noticed that you can use the url to get builds for any role on any champ, for example https://www.op.gg/champion/teemo/statistics/support/build will show some builds even tho the only role is top.

If so, making the window get on top with a "RIP CHAMPION" message, indicating to select a role to set would be an option. In the case of a champ with a role that has 0 data, the web still exists but has 0 elements, like Janna adc https://www.op.gg/champion/janna/statistics/adc/build so I think you can play around that info, but at that point you would have to check whats more optimal, since showing a drop down of roles with the pickrate % would mean entering 5 urls to fetch, and after selecting it, fetching again. And all in like 30 seconds (if you are last pick and took you some time to decide which one to lock, idk how much time do you have but not to much for sure)

Its kind of a problem to be fair 🤔

Btw if you can, add the abilities order to the in game build, very useful with some junglers that level up abilities in different order from level 1 to 3

jaeha-choi commented 2 years ago

Can you make the DFF window get on top of everything in case of errors or the lack of builds?

This is a tricky one. Last time I checked, the UI toolkit used in this project does not support always-on-top windows. It seems like there is an alternative solution, which is to use RequestFocus() instead.

I'm thinking of switching to GTK 3, which provides a function for always-on-top so that could be one other solution. But since this isn't the priority, it might take a while to implement this solution.

I noticed that you can use the url to get builds for any role on any champ, for example https://www.op.gg/champion/teemo/statistics/support/build will show some builds even tho the only role is top.

That's a good insight. It shows that data can still be pulled even if it's not the most popular choice.

In the case of a champ with a role that has 0 data, the web still exists but has 0 elements, like Janna adc https://www.op.gg/champion/janna/statistics/adc/build so I think you can play around that info, but at that point you would have to check whats more optimal, since showing a drop down of roles with the pickrate % would mean entering 5 urls to fetch, and after selecting it, fetching again. And all in like 30 seconds (if you are last pick and took you some time to decide which one to lock, idk how much time do you have but not to much for sure)

The good news is that the pick rate of every role is actually fetched within a single page. So what I could do is to show 0% pick rate for unpopular picks, and let the users choose those roles in case they really wanna try something unpopular.

Btw if you can, add the abilities order to the in game build, very useful with some junglers that level up abilities in different order from level 1 to 3

Yeah I think this is a great idea. Initially, I was working on this feature but I stopped because it lacks a unique property. I can still make it work tho.

ThePridestalker commented 2 years ago

Yeah I definitely meant the request focus not having it on top all the time.

ThePridestalker commented 2 years ago

Random idea I got, just dropping it here to see what you think.

Since the app doesnt uses an api and is fetching data per request, at start, it could fetch every single champ in the list https://www.op.gg/champion/statistics with the roles, and save every detail locally to make it super fast and avoid requesting to opgg too much.

It could download all the data on first start, and on every next starts, it could check if there are more champs or if the patch changed in opgg to make sure they have new statistics to download. Or add a button that does that on command like the "check updates".

I dont know how that first masive fetch would work to be progressive to avoid request restrictions, but would help to make every UI detail faster and more reliable. At the same time you wouldnt be able to choose stats for RIP champs or champs in weird roles with very very low pick rate. It could detect the champ im using and give me the runes for the lanes available, and if you want to play a different role, the build will be 99% the same unless you are jungle or support that changes your starter item. And you still can tweak runes, or maybe add a button to pause the "pool interval" if you dont want to close the app for a game(?)

What do you think? Is it too crazy of an idea? Is it more optimal or reliable? Is it even a good idea? 🤔

jaeha-choi commented 2 years ago

It's definitely an interesting idea. As you've mentioned, polling data for all champions might be a little heavy on their end. I.e. most people won't play more than 30 champions, so it could be a bit wasteful to request data for 150+ champions. But that said, I like the idea of storing the data to improve execution time. I think what can be done is to cache data once we poll it and save it until the next patch, etc. It's just a matter of when to renew the data: every patch? or every week? etc.

ThePridestalker commented 2 years ago

Thats true, you wont pick every champ, and we havent talk about ARAM or URF too, would make it 150+ x3 requests.

Maybe show a window with a list of champs to check, and request them slowly enough to not get timed out, idk.

About when to renew the data, I dont know how often does op.gg renews it, but on every champ, and in the /champion/statistics web, there is a class called champion-stats-header-version in the html, showing in the top right the version. It could automatically check if its different than the one we have saved locally, but I would also add a manual renew button since we dont really know how often do they update.

jaeha-choi commented 2 years ago

I'll see what I can do with caching. I'm just a little worried about giving the freedom to allow users from requesting a lot of data at once, even with the delay. (Current approach requires "lock-in" and this prevents spamming up to some degree)

If you don't mind me asking, how long does it usually take for your DFF to set up everything? Mine usually takes less than about 1~3 seconds, so I wasn't too worried about the latency until now.

Anyhow, I think caching is a great idea and I'll try to work on it once I'm done with fixing panic and logger issues.

ThePridestalker commented 2 years ago

True its not good to allow requesting whenever, maybe add a cooldown like the renew "Update" button of opgg in someones profile, that after renewing it doesnt let you request again for 2 minutes or something like that.

DFF takes 3 seconds for me, but if I change the role it takes 3 seconds again i think, not sure if I change only runes.

No rush on anything, thanks for the hard work! 👏

jaeha-choi commented 2 years ago

I was able to work on fixing some of the bugs you've mentioned, and also adding cache. I'm planning on fixing the "RIP" champion issues along with adding skill tree information, but in the meantime, feel free to try out the beta version. Since this is a pre-release, the update button in DFF won't work, and you'll need to visit release page and update it yourself.

jaeha-choi commented 2 years ago

I finished working on fixing the "RIP" champion issues as well as the skill tree implementation. It should be good to go 😃. The beta version might cause an issue because of cache, so make sure to delete the cache folder once you update DFF. You shouldn't have to delete the folder yourself after this update (hopefully).

ThePridestalker commented 2 years ago

How does the cache works? Do you save data once you have played the champ previously?

jaeha-choi commented 2 years ago

Once you lock in the champion, the data gets cached for that position (one item page + one spell set + four rune pages). If you request a different position, that gets cached too. Every cache is valid for 7 days or until the game/DFF updates.