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 Wrathful Raptor's ability trigger #11961

Merged
merged 2 commits into from
Mar 23, 2024

Conversation

jimga150
Copy link
Contributor

Continuation of work done in #11841

This fixes wrathful raptor's custom ability so that it only triggers once when dealt damage simultaneously by multiple sources at the same time.

I intend to use the changes approved here as a template for fixing any cards or abilities that use DAMAGED_PERMANENT as a trigger when they should be using DAMAGED_BATCH_FOR_ONE_PERMANENT, like Arcbond and DealtDamageAttachedTriggeredAbility.

Regarding fetching the target ID of the batch damage event: right now my usage of (UUID) CardUtil.getEventTargets(event).toArray()[0] feels a little spaghetti. I was thinking of adding a getTargetId() getter for DamagedBatchForOnePermanentEvent that would just call the former (or something equivalent) under the hood, since we can safely assume that the target ID list for this event should always be exactly one. It also has the added benefit of allowing people to just call the same getter as a single damage event since that makes sense intuitively to me. Thoughts?

@jimga150 jimga150 changed the title Fix Wratful Raptor's ability trigger Fix Wrathful Raptor's ability trigger Mar 18, 2024
@github-actions github-actions bot added the cards label Mar 18, 2024
@xenohedron xenohedron self-requested a review March 18, 2024 03:14
@xenohedron
Copy link
Contributor

I agree that it would be cleaner to just use getTargetId(). This gets into the whole structure of BatchGameEvent and DamagedBatchEvent, it's been expanded piecemeal resulting in both duplicated code and also inherited code that isn't relevant for all usages. I haven't decided on the best approach to streamline. For now, I suppose overriding the original method is acceptable.

Also, rather than toArray()[0], it's better to use a Stream and findFirst(), something like

                .stream()
                .findFirst()
                .map(GameEvent::getTargetId)
                .orElse(null)

Copy link
Member

@JayDi85 JayDi85 left a comment

Choose a reason for hiding this comment

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

was thinking of adding a getTargetId()

Do not add it in batches, only single override here (event type). Old methods raise errors by design, so devs will see wrong usage on copy-pasted or typed code (batch contains multiple events, so it must be processed as a list).

}

@Override
public boolean checkTrigger(GameEvent event, Game game) {
Permanent dinosaur = game.getPermanent(event.getTargetId());
Permanent dinosaur = game.getPermanent((UUID) CardUtil.getEventTargets(event).toArray()[0]);
Copy link
Member

@JayDi85 JayDi85 Mar 18, 2024

Choose a reason for hiding this comment

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

Use stream.first.orelse(null), not arrays.

@jimga150
Copy link
Contributor Author

@xenohedron this probably got buried in your notifications but it looks like your review is the blocker on this issue currently. Wouldn't normally ping for this, but you just responded to my other new PR today

@xenohedron
Copy link
Contributor

xenohedron this probably got buried in your notifications but it looks like your review is the blocker on this issue currently. Wouldn't normally ping for this, but you just responded to my other new PR today

Yeah, I manage quick turnarounds often, but sometimes I don't have time to get through everything until later. If it's been less than a week I most likely haven't forgotten.

Anyway, I haven't had time to plan out how to improve the structure the batch event hierarchy yet.

So I'm fine to approve this one, but rather than make similar changes by PR yet, can you first make an issue summarizing the scope of work? I'd prefer to tidy things up first so that the changes can be streamlined.

@xenohedron xenohedron merged commit 4787462 into magefree:master Mar 23, 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.

None yet

3 participants