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

[core] Allow targetfind to reject matching allegiances for ENEMY search #5628

Merged

Conversation

WinterSolstice8
Copy link
Member

@WinterSolstice8 WinterSolstice8 commented May 5, 2024

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?

This fixes an issue where due to some odd conditions Yagudo could sweep themselves, stun themselves, and break their states causing a crash.
Fixes #5618

Steps to test these changes

See #5618
Also test bomb toss suicide !exec target:useMobAbility(592) and see it works
Test moonlight, curaga, firaga, see them all work.

Xaver-DaRed
Xaver-DaRed previously approved these changes May 5, 2024
@WinterSolstice8 WinterSolstice8 added the hold On hold, pending further action/info label May 5, 2024
@WinterSolstice8
Copy link
Member Author

This touches a lot of stuff, gotta make sure it doesn't break anything, added hold label

zach2good
zach2good previously approved these changes May 5, 2024
@WinterSolstice8
Copy link
Member Author

Yup looks like some stuff broke because targetflags != findflags. it fixed my exact bug but some AoEs stopped working. Time to add targetflags into targetfind

We already do some jank conversions from targetflags -> findflags, so makes me wonder if it just should have been this way to begin with...

@WinterSolstice8 WinterSolstice8 force-pushed the fix_jank_as_hell_targetfind_bug branch 3 times, most recently from aad074c to c283121 Compare May 6, 2024 00:07
@WinterSolstice8 WinterSolstice8 dismissed stale reviews from Xaver-DaRed and zach2good May 6, 2024 00:07

Updated code

@WinterSolstice8 WinterSolstice8 changed the title [core] Ask mobs/players for target validity in TargetFind [core] Allow targetfind to reject mismatching allegiances for ENEMY search May 6, 2024
@WinterSolstice8 WinterSolstice8 force-pushed the fix_jank_as_hell_targetfind_bug branch from c283121 to fa70a4f Compare May 6, 2024 00:11
@WinterSolstice8 WinterSolstice8 changed the title [core] Allow targetfind to reject mismatching allegiances for ENEMY search [core] Allow targetfind to reject matching allegiances for ENEMY search May 6, 2024
@WinterSolstice8 WinterSolstice8 removed the hold On hold, pending further action/info label May 6, 2024
@WinterSolstice8
Copy link
Member Author

Alright, go ahead and re-review. had to touch more stuff

@@ -531,7 +531,7 @@ void CMobController::CastSpell(SpellID spellid)
{
// chance to target party
PMob->PAI->TargetFind->reset();
PMob->PAI->TargetFind->findWithinArea(PMob, AOE_RADIUS::ATTACKER, PSpell->getRange());
PMob->PAI->TargetFind->findWithinArea(PMob, AOE_RADIUS::ATTACKER, PSpell->getRange(), 0, 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use the enum values here instead of 0s please?

FINDFLAGS_NONE etc

Copy link
Member Author

Choose a reason for hiding this comment

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

there isnt an enum for targettype value 0

Copy link
Member Author

Choose a reason for hiding this comment

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

enum TARGETTYPE
{
    TARGET_SELF                    = 0x01,
    TARGET_PLAYER_PARTY            = 0x02,
    TARGET_ENEMY                   = 0x04,
    TARGET_PLAYER_ALLIANCE         = 0x08,
    TARGET_PLAYER                  = 0x10,
    TARGET_PLAYER_DEAD             = 0x20,
    TARGET_NPC                     = 0x40, // an npc is a mob that looks like an npc and fights on the side of the character
    TARGET_PLAYER_PARTY_PIANISSIMO = 0x80,
    TARGET_PET                     = 0x100,
    TARGET_PLAYER_PARTY_ENTRUST    = 0x200,
    TARGET_IGNORE_BATTLEID         = 0x400, // Can hit targets that do not have the same battle ID
};

Copy link
Contributor

Choose a reason for hiding this comment

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

No reason you can't add FINDFLAGS_NONE, rather than piping in a raw number that means the same thing

Copy link
Contributor

Choose a reason for hiding this comment

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

or TARGET_NONE, whatever

Copy link
Contributor

Choose a reason for hiding this comment

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

Then the storage and the argument types become the enum:

    FIND_TYPE m_findType;
    FINDFLAGS    m_findFlags;   // what to search for
    TARGETTYPE    m_targetFlags; // targetflags to reject potentially bad targets

Copy link
Member Author

Choose a reason for hiding this comment

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

Then the storage and the argument types become the enum:

    FIND_TYPE m_findType;
    FINDFLAGS    m_findFlags;   // what to search for
    TARGETTYPE    m_targetFlags; // targetflags to reject potentially bad targets

Apparently implementing this is editing a ton of stuff and I don't really have time to test every single permutation of spell/ability/mobskill/petskill right now

I've made the other changes asked of, though.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, that one was a "we should probably do this at some point" thing, currently looks good 👍

@WinterSolstice8 WinterSolstice8 force-pushed the fix_jank_as_hell_targetfind_bug branch from fa70a4f to a6c1aca Compare May 8, 2024 01:50
@WinterSolstice8 WinterSolstice8 force-pushed the fix_jank_as_hell_targetfind_bug branch from a6c1aca to df8ce3c Compare May 8, 2024 01:52
@zach2good zach2good merged commit bf6d94a into LandSandBoat:base May 8, 2024
11 checks passed
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.

🐛 [CRASH] Server crashes when a player gets charmed while a mob is readying Sweep
4 participants