magefree / mage

Magic Another Game Engine
http://xmage.today
MIT License
1.88k stars 770 forks source link

Issue with conditional "must be blocked if able" #11432

Closed permadeath closed 11 months ago

permadeath commented 11 months ago

I've found an issue with [[Frodo Baggins]] and [[Enkira, Hostile Scavenger]]. Both must be blocked if able if a condition is met. Creatures that must unconditionally be blocked if able function properly (I've tested [[Canopy Stalker]] and [[Gaea's Protector]] and they work as they should), but with the conditional ones, the opponent can still choose not to block even if the condition is met. I'm quite new to Xmage development so there may be something I am missing in the code, but looking at it, I don't see why it shouldn't work. They all use

MustBeBlockedByAtLeastOneSourceEffect(Duration.WhileOnBattlefield)

with Frodo Baggins and Enkira, Hostile Scavenger calling that within a ConditionalRequirementEffect(). The conditions seem to be correct for both cards.

SourceIsRingBearerCondition.instance

for Frodo Baggins and

EquippedSourceCondition.instance

for Enkira, Hostile Scavenger. Does anyone know what the issue could be and if so, what would be the solution?

github-actions[bot] commented 11 months ago

Frodo Baggins - (Gatherer) (Scryfall) (EDHREC)

{G}{W} Legendary Creature — Halfling Scout 1/3 Whenever Frodo Baggins or another legendary creature enters the battlefield under your control, the Ring tempts you. As long as Frodo is your Ring-bearer, it must be blocked if able.

Enkira, Hostile Scavenger - (Gatherer) (Scryfall) (EDHREC)

{3}{B}{G} Legendary Creature — Human Warrior 3/3 When Enkira, Hostile Scavenger enters the battlefield, create two Walker tokens. (They're 2/2 black Zombie creatures.) As long as Enkira is equipped, it must be blocked if able. Whenever Enkira and at least two Zombies attack, Enkira gains indestructible until end of turn.

Canopy Stalker - (Gatherer) (Scryfall) (EDHREC)

{3}{G} Creature — Cat 4/2 Canopy Stalker must be blocked if able. When Canopy Stalker dies, you gain 1 life for each creature that died this turn.

Gaea's Protector - (Gatherer) (Scryfall) (EDHREC)

{3}{G} Creature — Elemental Warrior 4/2 Gaea's Protector must be blocked if able.

xenohedron commented 11 months ago

Is it the AI that is incorrectly avoiding the restriction, or can a human player choose to avoid the restriction as well?

Can you confirm that ConditionalRequirementEffect() does function correctly for some other card?

permadeath commented 11 months ago

I can confirm that human players are able to avoid the restriction. I just tested out [[Reckless Cohort]], which uses ConditionalRequirementEffect() for its "attacks each combat if able unless you control another Ally" effect and it works properly.

Edit: Although, looking at Reckless Cohort, I see that the ConditionalRequirementEffect() is implemented a little differently. I'll see if I can try out that implementation with the bugged cards.

github-actions[bot] commented 11 months ago

Reckless Cohort - (Gatherer) (Scryfall) (EDHREC)

{1}{R} Creature — Human Warrior Ally 2/2 Reckless Cohort attacks each combat if able unless you control another Ally.

permadeath commented 11 months ago

Alright, so implementing it as Effect effect = new ConditionalRequirementEffect(...) with this.addAbility(new SimpleStaticAbility(Zone.BATTLEFIELD, effect) like in Reckless Cohort doesn't make a difference (although that may have seemed obvious to someone with more Java experience).

But I did replace the MustBeBlockedByAtLeastOneSourceEffect(Duration.WhileOnBattlefield) with AttacksIfAbleSourceEffect(Duration.WhileOnBattlefield, true) (which is Reckless Cohort's conditional ability) on both bugged cards, and that does work as intended, so it's specifically MustBeBlockedByAtLeastOneSourceEffect(Duration.WhileOnBattlefield) that is not working properly within ConditionalRequirementEffect()

xenohedron commented 11 months ago

Thanks for the research. I think the problem is that ConditionalRequirementEffect is missing the override for getMinNumberOfBlockers() (and some other non-abstract methods in RequirementEffect)

permadeath commented 11 months ago

I added the override for mustBlockAttackerIfElseUnblocked() to ConditionalRequirementEffect and it fixed the bug for both cards. Do you think that's sufficient, or should getMinNumberOfBlockers() be added as well?

xenohedron commented 11 months ago

I think all the overrides should be added to prevent potential similar bugs

permadeath commented 11 months ago

Excellent. I do have one Java noob question, though, for getMinNumberBlockers() since it's the only method that doesn't take parameters and the way it's used in MustBeBlockedByAtLeastOneSourceEffect is by declaring a private int outside the method. So would this work for the override in ConditionalRequirementEffect?

    @Override
    public int getMinNumberOfBlockers() {
        if (conditionState) {
            return effect.getMinNumberOfBlockers();
        } else if (otherwiseEffect != null) {
            return effect.getMinNumberOfBlockers();
        }
        return 0;
    }

Thanks a lot for your help, by the way.

xenohedron commented 11 months ago
    @Override
    public int getMinNumberOfBlockers() {
        if (conditionState) {
            return effect.getMinNumberOfBlockers();
        } else if (otherwiseEffect != null) {
            return otherwiseEffect.getMinNumberOfBlockers();
        }
        return super.getMinNumberOfBlockers();
    }
ssk97 commented 11 months ago

Conditional must-be-blocked effects have had a lot of problems in the past. The "what blocks are possible" code is a bit of a mess. I know we've previously had situations where it was impossible to continue since an attacking creature must be blocked while simultaneously being unblockable for some reason, preventing any combat from continuing.

xenohedron commented 11 months ago

See #10374 as one example. I think that's a separate issue from what's being fixed here though.

ssk97 commented 11 months ago

See #10374 as one example. I think that's a separate issue from what's being fixed here though.

You're right, I misread. These are "must be blocked if X" not "must be blocked by X". The later has problems, the former should be able to work once the ConditionalRequirementEffect override you mentioned is added.