Skip to content
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

Fix bugs for #3943 #4025

Closed
wants to merge 3 commits into from
Closed

Fix bugs for #3943 #4025

wants to merge 3 commits into from

Conversation

alexyaoyang
Copy link
Contributor

@alexyaoyang alexyaoyang commented Aug 15, 2016

Improve #3943

Improvements:

  • Changed references of walk to walk_min in follow path and spiral workers.
  • Swap walk_min and walk_max if walk_min > walk_max

@mention-bot
Copy link

@alexyaoyang, thanks for your PR! By analyzing the annotation information on this pull request, we identified @TheSavior, @leanhdaovn and @binarydepartment to be potential reviewers

@BriceSD
Copy link
Contributor

BriceSD commented Aug 15, 2016

Why walk_max and not walk_min ?
Having both seems more appropriate to me : self.bot.config.walk_max > 0 and self.bot.config.walk_min > 0

@BriceSD
Copy link
Contributor

BriceSD commented Aug 15, 2016

@mjmadsen Please review, need to fix asap.

@Setsu-BHMT
Copy link

Setsu-BHMT commented Aug 15, 2016

The intention of the original code was to check if the bot was supposed to walk at all; given that interpretation it might be more appropriate to check walk_min, though it's probably a good idea to rethink the logic here for the long run. Checking both shouldn't be necessary as logically if walk_min > 0 then walk_max must be greater than 0; this is assuming of course that we have a check somewhere to make sure walk_max is greater than or equal to walk_min (do we have this?)

@alexyaoyang
Copy link
Contributor Author

Yup, from what I can see, those if condition checks if user's config allows the bot to walk at all.

Thus checking for one of them should be sufficient.

@Setsu-BHMT You are right, I just tried, we do not have that check in place. I'll add a swap if min > max.

@BriceSD
Copy link
Contributor

BriceSD commented Aug 15, 2016

#4021

@douglascamata
Copy link
Member

The bot should not support teleporting anymore, as it will result is super fast ban. We should only support walking "normally", so those checks can be entirely removed, imho.

@alexyaoyang
Copy link
Contributor Author

@douglascamata I think checking if walking speed is > 0 is not related to teleporting?

@douglascamata
Copy link
Member

@alexyaoyang walk speed should be required and bot should use the default value of 4.16 if it's not provided in the config. If walk speed is 0, we need to bump it up to 4.16.

@alexyaoyang
Copy link
Contributor Author

@douglascamata I'm not sure if we are on the same issue here.

One issue is checking if walk_min is > 0 in the respective follow walkers, I would recommend this condition check to stay put.

Another issue is to disable teleporting, which I think is not related to this PR.

Final issue is bumping of walking speed of 0, which I have just committed. I took the liberty to randomize the bumped up walking speed between [3.0,5.0)

@mjmadsen
Copy link
Contributor

#4046 Didn't see this PR before posting. Either PR should suffice.

@elicwhite
Copy link
Contributor

I merged #4046. I'm really surprised this one doesn't look like it has merge conflicts. I'll let you close this one if you feel like the other was sufficient.

@alexyaoyang
Copy link
Contributor Author

Sure, closing this PR right now.

@CyberMew
Copy link

Maybe some of the changes not present in the other PR could be picked over.

@alexyaoyang
Copy link
Contributor Author

@CyberMew I'll create a new PR for the changes not present in #4046.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants