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

Rework Battlefield Group Death Counter #6223

Merged
merged 1 commit into from
Sep 7, 2024

Conversation

jmcmorris
Copy link
Contributor

I affirm:

  • I understand that if I do not agree to the following points by completing the checkboxes my PR will be ignored.
  • I understand I should leave resolving conversations to the LandSandBoat team so that reviewers won't miss what was said.
  • I have read and understood the Contributing Guide and the Code of Conduct.
  • I have tested my code and the things my code has changed since the last commit in the PR and will test after any later commits.

What does this pull request do?

Before this change it would track the number of mobs that have died in a battlefield group. Once all the mobs have died it would call the allDeath callback. This works fine for mobs that are spawned at the start of the battlefield and have to die. This does not work if not all mobs are spawned at the start and/or can be spawned multiple times and consequently the death count can reach the target amount without having all mobs dead.

This is fixed by tracking how many mobs are dead for the group when a mob dies.

Steps to test these changes

I'm not sure if we have any battlefields that currently work like this but I have one in the works (Call To Arms ISNM) that does. You can cheat this though by going to a battlefield that does have an allDeath and respawn a mob after killing it to see that it doesn't work without this change and will work with this change.

Divine Punishers BCNM has allDeath
!additem 1130
!zone Balgas Dias
Enter Divine Punishers and go kill a mob with !hp 0 after they despawn then respawn it. Kill the other mobs one by one and the bcnm should end with one mob remaining.

@WinterSolstice8
Copy link
Member

I think CI wants you to remove the whitespace below that brace, it has difficulty displaying removing whitespace in actions

@jmcmorris jmcmorris force-pushed the battlefield_alldeath branch 2 times, most recently from 5697fad to d297d6c Compare September 7, 2024 03:26
@jmcmorris jmcmorris force-pushed the battlefield_alldeath branch from d297d6c to b2fc307 Compare September 7, 2024 03:29
Copy link
Member

@WinterSolstice8 WinterSolstice8 left a comment

Choose a reason for hiding this comment

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

LGTM but want Clay's input

Copy link
Contributor

@claywar claywar left a comment

Choose a reason for hiding this comment

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

Seems reasonable to me assuming all else works. Was scratching my head thinking about broken mechanics, since I knew that I implemented some fights that had reraise mechanics. Looking at it, there's nothing that quite encountered this scenario yet.

Undying Promise has a reraise mechanic, but only one mob per group in the final set which does reraise. In this case, a local is tracking it, so should be no impact.

The other cases are more around pet respawn mechanics (such as Divine Might, AA battles), but those aren't in the required group.

@claywar claywar merged commit c08c2c4 into LandSandBoat:base Sep 7, 2024
12 checks passed
@jmcmorris jmcmorris deleted the battlefield_alldeath branch September 8, 2024 02:11
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