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

feat(core): OnFfaPvpStateUpdate Event #13023

Merged
merged 3 commits into from
Sep 21, 2022

Conversation

DavuKnight
Copy link
Contributor

Changes Proposed:

  • Add the OnFfaPvpStateUpdate Event so Lua Scripts can know when a user enters or leaves Ffa Pvp.

Issues Addressed:

  • Closes

SOURCE:

Tests Performed:

  • Builds Fine. There should be no Client Impact thats visible and testable.

How to Test the Changes:

55Honey has a direct need for this event in a current LuaScript and will Likely be the one to test it.

How to Test AzerothCore PRs

When a PR is ready to be tested, it will be marked as [WAITING TO BE TESTED].

You can help by testing PRs and writing your feedback here on the PR's page on GitHub. Follow the instructions here:

http://www.azerothcore.org/wiki/How-to-test-a-PR

REMEMBER: when testing a PR that changes something generic (i.e. a part of code that handles more than one specific thing), the tester should not only check that the PR does its job (e.g. fixing spell XXX) but especially check that the PR does not cause any regression (i.e. introducing new bugs).

For example: if a PR fixes spell X by changing a part of code that handles spells X, Y, and Z, we should not only test X, but we should test Y and Z as well.

@Yehonal Yehonal added CORE Related to the core file-cpp Used to trigger the matrix build labels Sep 14, 2022
@DavuKnight
Copy link
Contributor Author

DavuKnight commented Sep 14, 2022 via email

@Nyeriah
Copy link
Member

Nyeriah commented Sep 15, 2022

Fine by me but the CI build is failing, please take a look at those errors

@55Honey 55Honey changed the title OnFfaPvpStateUpdate Event feat(core): OnFfaPvpStateUpdate Event Sep 16, 2022
@55Honey
Copy link
Member

55Honey commented Sep 16, 2022

I don't understand why we fire the hook if a creature and a game object change their FfaPvP state. Do they have that state at all? Wouldn't it suffice to check just for players? Or I do read the code wrong maybe.
The way I read it, the hook can pass on a gameobject, creature or player.

@DavuKnight
Copy link
Contributor Author

I did it everywhere I saw the UnitPVPStateFlags.UNIT_BYTE2_FLAG_FFA_PVP Byte being set.

It should be Player and Creature. The Creature is to account for Pets. But I didnt dig into the why of someone trying to set FFA_PVP I just set the event up for any place the UNIT_BYTE2_FLAG_PVP was being set. The only change to the when/why or how its being set. Was that It now checks it before setting it. This is so the event only fires when the stat actually changes not whenever someone updates a state to the same state its already in.

Looking at the call in GameObject thats a Spell handler I presume it ensures that your Per Enters FFA_PVP as soon as you summon it when necessary.

@55Honey
Copy link
Member

55Honey commented Sep 16, 2022

Tested with azerothcore/mod-eluna#63
image

@ghost ghost added the To Be Merged label Sep 18, 2022
@acidmanifesto
Copy link
Contributor

Thank you for the PR

@acidmanifesto acidmanifesto merged commit d8598c7 into azerothcore:master Sep 21, 2022
@Nefertumm
Copy link
Member

@DavuKnight
Copy link
Contributor Author

Looking through the crash log Im not understanding the connection. Is there a specific Area of the log that assiciates the crash to this change? Or is it just a timing thing?

Comment on lines +2090 to +2094
if (!HasByteFlag(UNIT_FIELD_BYTES_2, 1, UNIT_BYTE2_FLAG_FFA_PVP))
{
sScriptMgr->OnFfaPvpStateUpdate(trigger, true);
trigger->SetByteFlag(UNIT_FIELD_BYTES_2, 1, UNIT_BYTE2_FLAG_FFA_PVP);
}
Copy link
Member

Choose a reason for hiding this comment

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

From the crashlog, this is related. I found it odd too :(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thread #19.2 is calling GameObject line 2092. 2092 is the event handler. Could it be something in an event payload in a lua script?
Should Event caLLs be wrapped in a try / catch / swallow to avoid crashing the server if the event overloads Blow out?

Copy link
Member

@Nefertumm Nefertumm Sep 22, 2022

Choose a reason for hiding this comment

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

I'm pretty sure there is no lua script running with that hook yet, although It's pretty odd indeed. It was a frost trap that made it crash, which is shown on the crashlog, but I don't have more information about it sadly :/
The event per-se it's okay, it doesn't need a try catch.
It's hitting the assert about the byte flag being larger than expected

Copy link
Contributor Author

@DavuKnight DavuKnight Sep 22, 2022

Choose a reason for hiding this comment

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

Seems to me the biggest change is the
ASSERT(offset < 4);
That HasByte Performs compared to the

    if (offset > 3)
    {
        LOG_ERROR("entities.object", "Object::SetByteFlag: wrong offset {}", offset);
        return;
    }

That SetByteFlag Does

Copy link
Contributor Author

Choose a reason for hiding this comment

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

azerothcore/mod-eluna#63 went in a few hours back not sure how quick the server picks those up

Copy link
Member

Choose a reason for hiding this comment

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

While the mod-Eluna PR is merged, there are no events registered yet in any Lua scripts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CORE Related to the core file-cpp Used to trigger the matrix build To Be Merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants