-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Revert "Avoid task init if task is disabled" (#5591) #5696
Conversation
I tend to agree with you on this one, however I noticed yesterday an issue with the way Sniper instantiates PokemonCatchWorker...
Passes Sniper's config to PokemonCatchWorker, which results in daily_catch_limit (and other values) being set to their defaults instead of reading from config. I added these print statements to show the issue. (note: daily_catch_limit in config is set to 2000)
So, I'm not sure the task disabling is actually the cause of this. |
That's exactly what I'm trying to fix.
When PokemonCatchWorker is initialized for the first time (and that should probably be before the I am not sure if the above approach is the best but definitely, again, we have to remove this global task initialization logic out of the TreeConfigBuilder/BaseTask. |
Well, instead of doing all those stuffs to PokemonCatchWorker, since multiple tasks want to share a common field, just move the Unless the PR in mention has broken something completely, I don't think you should be reverting it. Moreover when the PR in mention actually does fix quite a few issues. |
It's not just daily_catch_limit though, it's all this stuff:
Can we get the bot to call PokemonCatchWorker with its own config instead of supplying a new, unrelated one? |
Annoyingly, tasks do not seem to persist their config, and are instead completely reinitialized every time they're called. In an ideal world, the tasks would not have to reinit every time. Maybe there's a bigger bug that we haven't spotted yet that causes this. |
I agree, we got to solve this problem at its roots. Reverting the PR will just be playing ping pong without solving the actual problem. |
Yeah, it seems we're calling tasks as:
This generates a new instance, instead of being able to reference an existing task/worker instance. This issue applies to sniper, movetomappokemon and catchpokemon. Not sure where the existing instance is kept... self.bot.workers... something? If we can reuse existing worker, we can set task.pokemon = pokemon, then call task.work() |
@alexyaoyang Reverting this PR will solve this trouble it has already caused. It should of have been dispatched on the other 2 tasks that were acting strangely. I warned many of you. |
OK I've got a fix for it. I have no doubt someone with more python experience can make it a bit more elegant, but it works. PR shortly. |
I'm not entirely sure that issue is related to PR #5591. I think we already had it, because of the way PokemonCatchWorker is called by sniper and movetomappokemon. I still approve reverting this change, as it has no real benefit other than to remove control from individual tasks. Actually, it makes it incredibly difficult to fix, since the config isn't stored anywhere if the CatchPokemon task is disabled. PR #5708 resolves the sniper/mtmp config issue, but ONLY if CatchPokemon is enabled. Approving revert. |
PR #5708 addresses that specific issue. |
Well, if you guys think so, go ahead. However I still think that if multiple tasks are sharing common config, that config shouldn't be in one of the task. It's just pure confusing. Moving them to global is a great and simple idea that fixes this problem we have, and it's not too hard to implement, just move them to global and handle them in pokecli.py. |
For now, revert this to fix problems it has caused. We can always revisit later. |
@alexyaoyang I agree, it IS confusing. CatchPokemon passes its config to PokemonCatchWorker, which is fine, but then Sniper and MTMP need to pass the correct config, and they don't. Should we have config section 'type':'PokemonCatchWorker' which contains these values instead of having them in CatchPokemon? That would let PokemonCatchWorker automatically load its own config, instead of relying on other tasks to do it. For now, PR #5708 has a fix for this, which makes PokemonCatchWorker try to load CatchPokemon config if no config supplied. |
As expected, I just came across with the following situation:
If the user wants to disable catching nearby pokemons ONLY and enables Sniper and/or MoveToMapPokemon, the PokemonCatchWorker will never be initialized. But what's so bad about this? Well, I need to store the
daily_catch_limit
value (PokemonCatchWorker task's config), since it'll be used from both Sniper/MTMP, but if that task is never initialized, I will never have this value.This is just ONE MORE case that proves why we shouldn't handle such logic in the TreeConfigBuilder/BaseTask, like I said, unless you guys can come up with something better (post below).
These are definitely not viable: