Skip to content

Conversation

cameronangliss
Copy link
Contributor

@cameronangliss cameronangliss commented Dec 5, 2024

There is no way to send pass in poke-env. Having DefaultBattleOrder as the default option for the orders in DoubleBattleOrder is misleading, and it really should be pass. The user could just provide DefaultBattleOrder to the DoubleBattleOrder constructor if they want one of the sides to be default, so this is a strict empowerment for the user.

Copy link
Owner

@hsahovic hsahovic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good change.
Can you add some unit tests?

Copy link

codecov bot commented Dec 15, 2024

Codecov Report

Attention: Patch coverage is 86.66667% with 2 lines in your changes missing coverage. Please review.

Project coverage is 85.10%. Comparing base (f458350) to head (62095ac).
Report is 128 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #659      +/-   ##
==========================================
+ Coverage   83.38%   85.10%   +1.72%     
==========================================
  Files          39       42       +3     
  Lines        3918     4404     +486     
==========================================
+ Hits         3267     3748     +481     
- Misses        651      656       +5     

@cameronangliss cameronangliss force-pushed the none-battleorder-is-pass branch 2 times, most recently from b4121f4 to abbd584 Compare December 26, 2024 08:02
@cameronangliss cameronangliss marked this pull request as draft December 26, 2024 08:13
@cameronangliss cameronangliss force-pushed the none-battleorder-is-pass branch from abbd584 to 0df3a58 Compare December 26, 2024 08:17
@cameronangliss
Copy link
Contributor Author

cameronangliss commented Dec 26, 2024

@hsahovic this is being blocked by bugs in poke-env not being able to handle Zoroark properly😞 all other tests pass. I guess the good news is that this PR exposes a bug, but the bad news is that Zoroark is a nightmare. @caymansimpson if you're interested in taking down one of the most infamous problems of all time, you're invited hahaha

@cameronangliss cameronangliss marked this pull request as ready for review December 26, 2024 08:38
@caymansimpson
Copy link
Contributor

haha I don't think it's worth our time to tackle Illusion; it involves keeping multiple possible battle states and integrating speed/move inferences into the core of poke-env, which if comprehensively done, will require 10K+ lines of code (my inferences work is 95% accurate at 6K lines; there are so many niche interactions).

@cameronangliss cameronangliss marked this pull request as draft December 30, 2024 02:43
@cameronangliss cameronangliss marked this pull request as ready for review December 30, 2024 02:53
if side["pokemon"]:
self._player_role = side["pokemon"][0]["ident"][:2]
self._update_team_from_request(side)
if self.player_role is not None:
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this change in this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, this was a little while ago, but if I remember right, this was to try and simplify the code and fix Zoroark bugs on our side of the battle specifically (since that's all the request tells us about, is our own side).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to be clear, it's a replacement for the code being removed a few lines further below.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I definitely could test taking it out real quick and see what happens, whether that breaks stuff.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, I remember it all now. So, yes, this is a necessary fix, and it is for Zoroark. This is also just a much easier code chunk to look at than what it's replacing. Basically, the request will always put the active pokemon in the first and second positions in the pokemon list, and although you could be worried that we aren't correctly filtering out fainted pokemon, if you look at the _get_active_pokemon method in double_battle.py (which is used to give the active_pokemon property), you'll see that it is filtered there, so we were actually double-filtering before.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've now moved this change to #666, as that is where it is more relevant.

@cameronangliss cameronangliss force-pushed the none-battleorder-is-pass branch from 8457112 to d43f66d Compare January 3, 2025 00:29
@cameronangliss cameronangliss force-pushed the none-battleorder-is-pass branch 2 times, most recently from a72226a to 178a572 Compare January 20, 2025 05:33
@hsahovic
Copy link
Owner

I think this accidentally got commits from #666?

@cameronangliss cameronangliss force-pushed the none-battleorder-is-pass branch from 5687f30 to 8ff5d7a Compare January 31, 2025 22:25
@cameronangliss cameronangliss changed the title Putting "pass" in message when battle order is None in DoubleBattleOrder More robust battle tracking Jan 31, 2025
@cameronangliss cameronangliss force-pushed the none-battleorder-is-pass branch from 1d27fbd to c80d8a2 Compare February 1, 2025 02:39
@cameronangliss cameronangliss reopened this Feb 1, 2025
@cameronangliss cameronangliss changed the title More robust battle tracking None input maps to "pass" instead of DefaultBattleOrder Feb 1, 2025
@cameronangliss cameronangliss marked this pull request as draft February 6, 2025 04:42
@cameronangliss cameronangliss marked this pull request as ready for review February 16, 2025 21:11
@hsahovic hsahovic merged commit e74e280 into hsahovic:master Feb 22, 2025
9 checks passed
@cameronangliss cameronangliss deleted the none-battleorder-is-pass branch February 22, 2025 22:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants