search

kneasle / wheatley

An AI for Ringing Room that can ring any number of bells to increase the scope of practices.
https://pypi.org/project/wheatley/
MIT License
15 stars 13 forks source link

Feature/192 rounds #199

Closed jrs1061 closed 3 years ago

jrs1061 commented 3 years ago

Should i set "self._rows_left_before_rounds = 1 or 0" i.e should it immediately go into rounds

centreboard commented 3 years ago

I think we should consider just setting self._is_ringing_opening_row = true

I haven't so far been at a practice where someone has needed to call Rounds in RingingRoom, but as a call of rounds IRL is often after a fireup, it might be worth calling self._rhythm.return_to_mainloop(). That would clear if Wheatley is currently waiting on a bell, though it wouldn't be sufficient to stop Wheatley waiting for other bells later in the row.

jrs1061 commented 3 years ago

Thanks Matt, I'll take another look. Maybe after work tomorrow

jrs1061 commented 3 years ago

We usually just call that's all.

kneasle commented 3 years ago

I think we should consider just setting self._is_ringing_opening_row = true

Yep I agree with this 100%.

I haven't so far been at a practice where someone has needed to call Rounds in RingingRoom, but as a call of rounds IRL is often after a fireup, it might be worth calling self._rhythm.return_to_mainloop(). That would clear if Wheatley is currently waiting on a bell, though it wouldn't be sufficient to stop Wheatley waiting for other bells later in the row.

Being completely honest, this has me very divided... on the one hand if someone calls Rounds then they probably want to restart the touch as fast as possible (i.e. return to the --start-row as fast as possible). However, I think calling Rhythm.return_to_mainloop part-way through a row is quite likely to result in the bells being at random strokes, which is probably not the desired effect.

So I think probably just wait for everyone to finish the current row (in whatever order) and then return immediately to the starting row.

jrs1061 commented 3 years ago

I've set the first suggestion. Without going into too much of the internals, I can see setting to rounds doesn't bring you back to the start which is the intention of 'rounds'

jrs1061 commented 3 years ago

If I set wheatley to rounds then I think the danger is wheatley could decide it's the end..

kneasle commented 3 years ago

I've set the first suggestion. Without going into too much of the internals, I can see setting to rounds doesn't bring you back to the start which is the intention of 'rounds'

I'm not quite sure what you mean here - the row generator gets reset every time Wheatley goes into changes, and if Bot._is_ringing_starting_row == True then Bot won't consume any more rows from the row generator and so the touch can't stop. If I'm correct and this is fine then I'm happy to merge this, but I may well not be so it's worth being sure. Also, if you could put something in the CHANGE_LOG about this PR then that'd be great - otherwise I'll do it just before merging so it's fine.

Sorry for the inactivity - the deadline for my degree project has just passed, and I have finals coming up...

jrs1061 commented 3 years ago

Completely understand - I'll take another look on Fri and check it should work as expected

jrs1061 commented 3 years ago

Having a look at the logic. I have some questions (bear of little brain)... I've added method: _on_rounds - that sets _is_ringing_opening_row = true then method: generate_next_row sets _row = _opening_row method: start_next_row if _rounds_left_before_method == 0 then it sets _is_ringing_opening_row to false

so I think _rounds_left_before_method should also be set to none

what do you think - I'm still going through the logic... So please bear with... I have my own RR set up so I can probably drop the code in to the lib in venv and test it

kneasle commented 3 years ago

so I think _rounds_left_before_method should also be set to none

This is definitely correct, but I think _rounds_left_before_method is already set to None when Wheatley initially goes into changes. In all honesty, this whole 'what is Wheatley doing?' system is a giant potential source of bugs - it should be a state machine (with states like RingingRounds, RingingMethod, Idle, etc.) but AFAIK there isn't a nice way to do this in Python.

what do you think - I'm still going through the logic... So please bear with... I have my own RR set up so I can probably drop the code in to the lib in venv and test it

You shouldn't need your own local RR server to test this out - but if you do want to test it by running Wheatley server-side then you can override where the server looks for Wheatley with the RR_WHEATLEY_PATH environment variable (see here). Setting this will override the copy installed by Pip.

kneasle commented 3 years ago

Actually, thinking about it, I'm not sure what'd happen currently in the edge case where someone calls Rounds! between Go and the start of the method (I have too much revision to check it out). In this case, AFAIK the counter is still counting down and Wheatley will continue into changes. So probably better safe than sorry and set _rounds_left_before_method in Bot._on_rounds.

jrs1061 commented 3 years ago

Thanks Ben - when you're free of study pressure we can go over this a bit more.

On Sat, 29 May 2021 at 16:25, Ben White-Horne @.***> wrote:

Actually, thinking about it, I'm not sure what'd happen currently in the edge case where someone calls Rounds! between Go and the start of the method (I have too much revision to check it out). In this case, AFAIK the counter is still counting down and Wheatley will continue into changes. So probably better safe than sorry and set _rounds_left_before_method in Bot._on_rounds.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/kneasle/wheatley/pull/199#issuecomment-850851070, or unsubscribe https://github.com/notifications/unsubscribe-auth/AF32XV4PIF3X46UICNZSHNTTQEBOTANCNFSM44PGGS4A .

kneasle commented 3 years ago

Thanks Ben - when you're free of study pressure we can go over this a bit more.

Cool! I'd be happy to merge this PR as it stands if Bot._on_rounds() sets self._rounds_left_before_method = None (perhaps with a comment about the Rounds after Go edge case)... I'm not 100% sure it's necessary but it seems like a wise move anyway. @centreboard, what do you reckon?

jrs1061 commented 3 years ago

I'm not happy with it... I'd like to poke around a little bit more.. If I can't do better - If you have stop at rounds it still does that. i.e. I think it should take you to where you'd be if you've just done look to

jrs1061 commented 3 years ago

The only thing I think I'm a bit unsure of is if stop at rounds is set and you call rounds it will just stop there. All other functionality I'm fine with and the stop at rounds is kinda what it's been told to do... So I'm happy with the current iteration

jrs1061 commented 3 years ago

If you feel it's fit foe merging please do

kneasle commented 3 years ago

Argh pylint is being dumb again... I'm honestly not sure what's up, because I've installed exactly the same version and run it on my PC and it works fine. The rogue build CI run is also still hanging around - I should probably ask the help peeps about it. But these are not actually an issue and I can merge regardless...

jrs1061 commented 3 years ago

I noticed pylint having an issue with both pull requests

centreboard commented 3 years ago

Argh pylint is being dumb again... I'm honestly not sure what's up, because I've installed exactly the same version and run it on my PC and it works fine. The rogue build CI run is also still hanging around - I should probably ask the help peeps about it. But these are not actually an issue and I can merge regardless...

pylint issue seems to have been fixed by updating it #203

Have you tried removing the required build check and adding it again? https://docs.github.com/en/github/administering-a-repository/defining-the-mergeability-of-pull-requests/managing-a-branch-protection-rule

jrs1061 commented 3 years ago

Argh pylint is being dumb again... I'm honestly not sure what's up, because I've installed exactly the same version and run it on my PC and it works fine. The rogue build CI run is also still hanging around - I should probably ask the help peeps about it. But these are not actually an issue and I can merge regardless...

pylint issue seems to have been fixed by updating it #203

Have you tried removing the required build check and adding it again? https://docs.github.com/en/github/administering-a-repository/defining-the-mergeability-of-pull-requests/managing-a-branch-protection-rule

Is that how to rerun the checks? I don't access to settings..

kneasle commented 3 years ago

Have you tried removing the required build check and adding it again? https://docs.github.com/en/github/administering-a-repository/defining-the-mergeability-of-pull-requests/managing-a-branch-protection-rule

Aha yes that's fixed it! Is everyone happy for this to be merged?

jrs1061 commented 3 years ago

I'm happy with it - it makes sense and seems to do what we want. - thanks again for all the help!

kneasle commented 3 years ago

I'm happy with it - it makes sense and seems to do what we want. - thanks again for all the help!

Awesome! Thanks for the work you've put into the PR - we really appreciate it :smiley:.