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 LegateLaniusCaesarsAce counting non-controlled creatures #12183

Merged
merged 1 commit into from Apr 30, 2024

Conversation

ssk97
Copy link
Contributor

@ssk97 ssk97 commented Apr 25, 2024

Fixes #12178

@github-actions github-actions bot added the cards label Apr 25, 2024
Copy link
Contributor

@Susucre Susucre left a comment

Choose a reason for hiding this comment

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

Oh right, Battlefield::count counts all the permanents matching the filter in the range of influence of the player. It is usually used with a FilterControlledPermanent, but there was no controller predicate here.

I think we could rename the method into one not having the ambiguity? But not sure how other think of it, maybe that's fine as is.

@xenohedron
Copy link
Contributor

I prefer using a filter with a controller predicate, and the count method that respects range of influence and checks ObjectSourcePlayerPredicates in the filter. The countAll method only calls filter.match with two arguments so is not best style, even though it usually doesn't cause any bugs (and yes, that is used all over the codebase, and fixing such usages would be a lot of work).

@Susucre Susucre merged commit 9f04062 into magefree:master Apr 30, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Legate Lanius, Caesar's Ace not asking for correct number of creatures to be sacrificed.
3 participants