chore(Core/AllMapScript) Update structure #19979

pangolp · 2024-09-15T22:04:28Z

Changes Proposed:

This PR proposes changes to:

Hooks

Issues Addressed:

Closes

Tests Performed:

This PR has been:

Tested in-game by the author.

How to Test the Changes:

When updating the crossfaction pve module, I tried to use the OnDestroyInstance hook and noticed that it was not being called. When checking the rest of the hooks, and remembering that the structure had been updated, I rewrote the files to adapt them and managed to get the hook to work in the module.
Basically, to test the pull request, you should use the hook without this change, and then use the hook with the change. In the first case, it should not be called, and in the second, it should execute without problems.

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.

pangolp · 2024-09-15T22:35:54Z

I don't know why the tests fail. Because the compilation finished 100% without problems.
Maybe it's a github actions issue or I don't know what's happening to try to fix it.

Yehonal · 2024-09-15T22:39:41Z

is the CALL_ENABLED_HOOKS missing in other files too? maybe the refactoring was not complete

pangolp · 2024-09-15T22:52:41Z

is the CALL_ENABLED_HOOKS missing in other files too? maybe the refactoring was not complete

In PlayerScript, I saw that it was modified, but not in this file, perhaps other files need to be modified.

pangolp · 2024-09-15T22:54:03Z

We could make a pull request to finish working on all of them, there would be no problem. It would take more time, I suppose, and some additional testing. Perhaps it would be better to divide the work and do it little by little, in case something had to be reverted. Also, if the change is very big, it is more difficult to test.

pangolp · 2024-09-15T22:55:24Z

Unless your comment refers to hooks inside these files, but which belong to other scripts, then yes, I would probably have to modify them, but I really could use an example, or someone else to help me by committing a commit on this same pull request.

Yehonal · 2024-09-15T23:00:49Z

What I'm saying is that if without this macro all the other hooks are not working, probably we should fix them all. But I'd like to ask the original author because I'm not sure what's the problem here and I don't want to make assumptions @Winfidonarleyan @walkline ?

pangolp · 2024-09-15T23:04:23Z

What I'm saying is that if without this macro all the other hooks are not working, probably we should fix them all. But I'd like to ask the original author because I'm not sure what's the problem here and I don't want to make assumptions @Winfidonarleyan @walkline ?

Sounds perfect to me. Let's wait for your reply then.

Winfidonarleyan · 2024-09-16T03:47:32Z

What I'm saying is that if without this macro all the other hooks are not

working, probably we should fix them all. But I'd like to ask the original

author because I'm not sure what's the problem here and I don't want to

make assumptions

@Winfidonarleyan @walkline ?

I don't know why not work. Anyway, I don't like this system with selected hooks.

sudlud · 2024-09-16T05:35:04Z

The implementation should be fully backwards compatible, e.g. enabling all hooks of a script if the enable-vector was not provided.

Also I would have expected that the AllMapScript should still have worked before this PR, as the changes are not necessary for it to work? It's just an optimization

walkline · 2024-09-16T07:48:56Z

Here is the original PR: #18672, where you can find details about the problem it aims to solve.

As @sudlud mentioned, the change is backward compatible, and AllMapScript should work as it is. However, without this change, it would remain unoptimized.

I’m not sure why the CI is crashing, and I can’t debug it at the moment. If someone could provide a crash log, that would be helpful.

Yehonal · 2024-09-16T08:04:35Z

Here is the original PR: #18672, where you can find details about the problem it aims to solve.

As @sudlud mentioned, the change is backward compatible, and AllMapScript should work as it is. However, without this change, it would remain unoptimized.

I’m not sure why the CI is crashing, and I can’t debug it at the moment. If someone could provide a crash log, that would be helpful.

Besides the CI crashing, why do you think @pangolp is experiencing this issue? If the old hooks are not working anymore, this is definitely a critical problem. Can someone double check and confirm this please?

@Winfidonarleyan why you don't like it? What's your opinion?

walkline · 2024-09-16T08:18:20Z

I don’t think there are any other issues, and I believe @pangolp used the wrong title (specifically the “fix” part) for this PR, as it is not addressing any bugs.

Yehonal · 2024-09-16T09:03:13Z

Seems @pangolp is stating that without the new macro, the hooks are not being called at all. Which sounds like a bug to me. But I might misunderstood it

pangolp · 2024-09-16T10:48:37Z

You can use the CFPVE module, where we use the hook, and you'll see that it doesn't work, unless this change is made. https://github.com/pangolp/mod-cfpve

pangolp · 2024-09-16T10:50:28Z

That doesn't mean, the title of the pull request, maybe it's wrong. But before this change, the hook didn't work for me and I leave you the module to try it yourself or invoke it, to see if it works for you in any module of your own.

pangolp · 2024-09-16T12:27:28Z

@Yehonal @Winfidonarleyan @walkline @sudlud When I started working on this part of the code specifically

class CfAllMapScript : public AllMapScript
{
public:
    CfAllMapScript() : AllMapScript("CfAllMapScript") { }

    void OnDestroyInstance(MapInstanced* /*mapInstanced*/, Map* map) override
    {
        QueryResult result = CharacterDatabase.Query("SELECT `factionID` FROM `mod_crossfaction_pve` WHERE `instanceID`={}", map->GetInstanceId());

        if (result)
        {
            CharacterDatabase.DirectExecute("DELETE FROM `mod_crossfaction_pve` WHERE `instanceID`={}", map->GetInstanceId());
        }
    }
};

Notice that it was not running. So reviewing the file, I saw that the structure of AllMapScript was not the same as that of PlayerScript, I made only 1 of the changes, so that the specific hook that I am using works for me and I saw that it was executed, That's why I decided to upload the pull request, so that you can tell me if it is correct, and if something needs to be corrected, also let me know, so we can do it and merge it.

Don't focus on the content of the module, because it is a temporary solution to a very specific bug that the crossfaction currently has, but there must be better ways to solve it, however, for now, we came up with that one.

And on the other hand, the title may not be correct, so if someone wants to adapt it, they can do so without problems, so that it is more representative of the problem I am trying to present and correct.

I look forward to any modifications and comments you make on this topic. Thanks in advance.

pangolp · 2024-09-16T12:29:12Z

In parallel, I would also like to know why the tests failed, why a compilation error, it seems that it is not. The fault seems to be elsewhere. @Helias, can you run the tests again please?, thank you.

Helias · 2024-09-16T13:56:03Z

done 👍

Yehonal · 2024-09-16T14:14:50Z

I do understand what you are doing here. What I do not understand is if these hooks were broken, most likely also the other ones that are not using the new CALL_ENABLED_HOOKS might be broken. This PR aims to fix a issue with a specific bug for your specific case, but our interest as AC devs here is to also understand if this is a general problem or not Can someone test it? Also, it would be great to understand the exact reason why it was not working if people are stating that the changes were backward compatible. I don't like to close this case and have possibly fixed/considered half of the problem and forget about the remaing

…

On Mon, Sep 16, 2024, 15:56 Stefano Borzì ***@***.***> wrote: done 👍 — Reply to this email directly, view it on GitHub <#19979 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABD5FAKIHKCLX3BZPHRNRTZW3PQTAVCNFSM6AAAAABOIF53TWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGNJSHE4TSMZVG4> . You are receiving this because you were mentioned.Message ID: ***@***.***>

pangolp · 2024-09-16T14:26:50Z

I do understand what you are doing here. What I do not understand is if these hooks were broken, most likely also the other ones that are not using the new CALL_ENABLED_HOOKS might be broken. This PR aims to fix a issue with a specific bug for your specific case, but our interest as AC devs here is to also understand if this is a general problem or not Can someone test it? Also, it would be great to understand the exact reason why it was not working if people are stating that the changes were backward compatible. I don't like to close this case and have possibly fixed/considered half of the problem and forget about the remaing

That seems fine to me. I am available to help with the solution.

sudlud · 2024-09-16T16:32:56Z

@PkllonG did you check your script before PR? So we know if there was actually an issue with allmapscript.

pangolp · 2024-09-16T20:50:39Z

@PkllonG did you check your script before PR? So we know if there was actually an issue with allmapscript.

I didn't use this PR, The module works fine.

Does it work without this pull request or did you have to use it to make it work?
Because I wanted to use the hook in the custom module that happened, and there was no case, until I applied this change.
You can do a std::cout or something similar, using the OnDestroyInstance to see if the hook call works please. Thank you.

sudlud · 2024-09-16T21:38:17Z

@Yehonal @Winfidonarleyan @walkline @sudlud When I started working on this part of the code specifically
class CfAllMapScript : public AllMapScript
{
public:
    CfAllMapScript() : AllMapScript("CfAllMapScript") { }

    void OnDestroyInstance(MapInstanced* /*mapInstanced*/, Map* map) override
    {
        QueryResult result = CharacterDatabase.Query("SELECT `factionID` FROM `mod_crossfaction_pve` WHERE `instanceID`={}", map->GetInstanceId());

        if (result)
        {
            CharacterDatabase.DirectExecute("DELETE FROM `mod_crossfaction_pve` WHERE `instanceID`={}", map->GetInstanceId());
        }
    }
};
Notice that it was not running. So reviewing the file, I saw that the structure of AllMapScript was not the same as that of PlayerScript, I made only 1 of the changes, so that the specific hook that I am using works for me and I saw that it was executed, That's why I decided to upload the pull request, so that you can tell me if it is correct, and if something needs to be corrected, also let me know, so we can do it and merge it.

Don't focus on the content of the module, because it is a temporary solution to a very specific bug that the crossfaction currently has, but there must be better ways to solve it, however, for now, we came up with that one.

And on the other hand, the title may not be correct, so if someone wants to adapt it, they can do so without problems, so that it is more representative of the problem I am trying to present and correct.

I look forward to any modifications and comments you make on this topic. Thanks in advance.

@pangolp the script you provided works fine on a35f0c4 (current master before this PR).

so @PkllonG and myself have verified that there has been no general issue to AllMapScript.

did you probably forget this on your first test runs:

new CfAllMapScript();

(because that's an issue i just ran into while testing your AllMapScript.)

pangolp · 2024-09-16T22:06:28Z

@Yehonal @Winfidonarleyan @walkline @sudlud When I started working on this part of the code specifically
class CfAllMapScript : public AllMapScript
{
public:
    CfAllMapScript() : AllMapScript("CfAllMapScript") { }

    void OnDestroyInstance(MapInstanced* /*mapInstanced*/, Map* map) override
    {
        QueryResult result = CharacterDatabase.Query("SELECT `factionID` FROM `mod_crossfaction_pve` WHERE `instanceID`={}", map->GetInstanceId());

        if (result)
        {
            CharacterDatabase.DirectExecute("DELETE FROM `mod_crossfaction_pve` WHERE `instanceID`={}", map->GetInstanceId());
        }
    }
};
Notice that it was not running. So reviewing the file, I saw that the structure of AllMapScript was not the same as that of PlayerScript, I made only 1 of the changes, so that the specific hook that I am using works for me and I saw that it was executed, That's why I decided to upload the pull request, so that you can tell me if it is correct, and if something needs to be corrected, also let me know, so we can do it and merge it.
Don't focus on the content of the module, because it is a temporary solution to a very specific bug that the crossfaction currently has, but there must be better ways to solve it, however, for now, we came up with that one.
And on the other hand, the title may not be correct, so if someone wants to adapt it, they can do so without problems, so that it is more representative of the problem I am trying to present and correct.
I look forward to any modifications and comments you make on this topic. Thanks in advance.
@pangolp the script you provided works fine on a35f0c4 (current master before this PR).

so @PkllonG and myself have verified that there has been no general issue to AllMapScript.

did you probably forget this on your first test runs:
new CfAllMapScript();
(because that's an issue i just ran into while testing your AllMapScript.)

It is included and I always used it, but hey, if they tried the hook and it worked, it would have been my problem I guess.
Could it perhaps have something to do with the operating system? I am using Windows.
It wouldn't be the first time there has been some incompatibility.
With the emails, and the server crash, it did not fail in Ubuntu, but in Windows it did.

void CfpveScripts()
{
    new CfPlayerScript();
    new CfAllMapScript();
    new CfServerScript();
}

Yehonal · 2024-09-17T00:36:49Z

Ok, thanks for checking. so, considering this, I have another question before approve/merge this PR:

Is the new CALL_ENABLED_HOOKS intended to be used everywhere? If so, this PR might still be worth merging anyway to keep the code up to date, but at the same time, there should be no priority in converting all the other hooks for now

What do you think

pangolp · 2024-09-17T04:49:10Z

I'm could keep working on this pull request. I just need to be told what to do, we change the title and we add more commits. I'm not in a hurry to merge it, it's not critical. But he needed some example maybe to finish the work, although I'll try to investigate something on my own. This is the first time I have made a change of this style.

sudlud · 2024-09-17T05:15:42Z

@Yehonal as @walkline mentioned, the enable-hooks-mechanic originated in #18627.

the majority of scripts has been converted already (see referenced PRs in #18627) and there have been no issues so far that I know of.

so the only remaining issue in this PR imo is the failing CI, maybe @pangolp can please merge latest master to ensure everything’s up to date?
It seems the worldserver is crashing on startup in the CI runs, which does not happen in any other of the currently open PRs, so this must be addressed imo.

Nyeriah · 2024-09-17T11:23:10Z

What is interesting is the module CI isn't crashing

pangolp · 2024-09-17T12:13:57Z

@Yehonal as @walkline mentioned, the enable-hooks-mechanic originated in #18627.

the majority of scripts has been converted already (see referenced PRs in #18627) and there have been no issues so far that I know of.

so the only remaining issue in this PR imo is the failing CI, maybe @pangolp can please merge latest master to ensure everything’s up to date?
It seems the worldserver is crashing on startup in the CI runs, which does not happen in any other of the currently open PRs, so this must be addressed imo.

I'm going to check. But I think there's work to be done here anyway. I will also review the pull request that you mention.

pangolp · 2024-09-17T12:14:41Z

What is interesting is the module CI isn't crashing

I'm going to review this as well. Thank you.

pangolp · 2024-09-17T12:21:18Z

I'll update the branch to see if the bugs persist, but then I'll have to look at the pull request and finish writing the macros I missed.
Sometimes the translator plays tricks on me and that's why, when something is not clear to me, I ask again. Because if I read it in a predetermined way, like this pull request is useless, but then they mention that they are possibly going to merge it and I don't understand anything hahaha. But it's the translator, who by not giving it context, confuses me a little.

😅

…otlk into mapScript

pangolp · 2024-09-17T13:40:06Z

Do you need me to test if OnDestroyInstance is running before PR?

Yes, please, I tried it and it didn't work for me. That's why I created this pull request.

sudlud · 2024-09-17T13:50:15Z

Do you need me to test if OnDestroyInstance is running before PR?

I already tested it, it works, see my message above. @pangolp please do not tell several people to test the same thing again and again

pangolp · 2024-09-17T14:46:53Z

I already tested it, it works, see my message above. @pangolp please do not tell several people to test the same thing again and again

I didn't do it on purpose. Again, since I use a translator, sometimes I can't follow the threads of the conversations or I forget, because I have to use the translator again. The person offered, and I said yes. But I didn't do it with bad intentions. You could put a tested label if not, and people would already know that it was tested.

pangolp · 2024-09-17T15:17:08Z

Provide a module for them to test. https://github.com/pangolp/mod-cfpve
Explain the reasons why I made the change (OnDestroyInstance did not work for me without this change)
I am available for any changes or improvements that you deem appropriate.
I'm in no hurry to merge it.

The only bad thing is that I can't fix the failure in the tests.
Because it's not about compilation.

pangolp · 2024-09-19T15:59:04Z

Does anyone know why this change is not passing the tests to try to fix it? Because it doesn't seem to be a compilation issue.

…otlk into mapScript

Nyeriah · 2024-09-20T11:06:40Z

This PR is in a stalemate. The CI is crashing on start up but (to my knowledge) we can't see the crash log produced. I have personally tested this PR and no crash is produced when starting up the server. We, however, cannot merge it with a broken CI.

pangolp · 2024-09-21T05:03:14Z

This PR is in a stalemate. The CI is crashing on start up but (to my knowledge) we can't see the crash log produced. I have personally tested this PR and no crash is produced when starting up the server. We, however, cannot merge it with a broken CI.

The question would be how to solve this problem...
What is causing it to break...

…otlk into mapScript

pangolp · 2024-09-21T15:38:31Z

If the hook works and since we can't correct the problem in the tests, I think we should close this.

walkline · 2024-09-21T17:49:47Z

It's crashing because you forgot to add AllMapScript here -

azerothcore-wotlk/src/server/game/Scripting/ScriptMgr.cpp

Line 104 in 386aea0

ScriptRegistry<WorldScript>::InitEnabledHooksIfNeeded(WORLDHOOK_END);

pangolp · 2024-09-21T17:57:17Z

It's crashing because you forgot to add AllMapScript here -

If that happens, I will travel personally and give you a hug haha.

…otlk into mapScript

Co-authored-by: Anton Popovichenko <[email protected]>

pangolp · 2024-09-21T19:12:01Z

@walkline You are the hero of the day.
@Nyeriah I've already passed the tests, all yours.

fix(Core/AllMapScript) Update structure to use CALL_ENABLED_HOOKS

a6212bb

github-actions bot added CORE Related to the core file-cpp Used to trigger the matrix build labels Sep 15, 2024

fatal error: no template named 'vector' in namespace 'std'

96e7a65

Helias requested a review from Yehonal September 15, 2024 22:21

Yehonal requested a review from Winfidonarleyan September 15, 2024 22:39

Merge branch 'master' of https://github.com/azerothcore/azerothcore-w…

3911f78

…otlk into mapScript

pangolp changed the title ~~fix(Core/AllMapScript) Update structure to use CALL_ENABLED_HOOKS~~ chore(Core/AllMapScript) Update structure to use CALL_ENABLED_HOOKS Sep 17, 2024

pangolp changed the title ~~chore(Core/AllMapScript) Update structure to use CALL_ENABLED_HOOKS~~ chore(Core/AllMapScript) Update structure Sep 17, 2024

Merge branch 'master' of https://github.com/azerothcore/azerothcore-w…

b2d8f0d

…otlk into mapScript

Merge branch 'master' of https://github.com/azerothcore/azerothcore-w…

dcf731b

…otlk into mapScript

pangolp and others added 2 commits September 21, 2024 14:59

Merge branch 'master' of https://github.com/azerothcore/azerothcore-w…

03aad56

…otlk into mapScript

fix build

adedc5a

Co-authored-by: Anton Popovichenko <[email protected]>

sudlud approved these changes Sep 21, 2024

View reviewed changes

sudlud added the To Be Merged label Sep 21, 2024

sudlud merged commit cfd7bf4 into azerothcore:master Sep 21, 2024
17 checks passed

pangolp deleted the mapScript branch September 22, 2024 04:42

chore(Core/AllMapScript) Update structure #19979

chore(Core/AllMapScript) Update structure #19979

Conversation

pangolp commented Sep 15, 2024 • edited Loading

Changes Proposed:

Issues Addressed:

Tests Performed:

How to Test the Changes:

How to Test AzerothCore PRs

pangolp commented Sep 15, 2024

Yehonal commented Sep 15, 2024

pangolp commented Sep 15, 2024

pangolp commented Sep 15, 2024

pangolp commented Sep 15, 2024

Yehonal commented Sep 15, 2024 via email • edited Loading

pangolp commented Sep 15, 2024

Winfidonarleyan commented Sep 16, 2024

sudlud commented Sep 16, 2024

walkline commented Sep 16, 2024

Yehonal commented Sep 16, 2024

walkline commented Sep 16, 2024

Yehonal commented Sep 16, 2024

pangolp commented Sep 16, 2024

pangolp commented Sep 16, 2024

pangolp commented Sep 16, 2024 • edited Loading

pangolp commented Sep 16, 2024

Helias commented Sep 16, 2024 • edited Loading

Yehonal commented Sep 16, 2024 via email

pangolp commented Sep 16, 2024

sudlud commented Sep 16, 2024

pangolp commented Sep 16, 2024 • edited Loading

sudlud commented Sep 16, 2024

pangolp commented Sep 16, 2024

Yehonal commented Sep 17, 2024 • edited Loading

pangolp commented Sep 17, 2024 • edited Loading

sudlud commented Sep 17, 2024

Nyeriah commented Sep 17, 2024

pangolp commented Sep 17, 2024

pangolp commented Sep 17, 2024

pangolp commented Sep 17, 2024 • edited Loading

pangolp commented Sep 17, 2024

sudlud commented Sep 17, 2024

pangolp commented Sep 17, 2024

pangolp commented Sep 17, 2024 • edited Loading

pangolp commented Sep 19, 2024

Nyeriah commented Sep 20, 2024

pangolp commented Sep 21, 2024

pangolp commented Sep 21, 2024 • edited Loading

walkline commented Sep 21, 2024

pangolp commented Sep 21, 2024

pangolp commented Sep 21, 2024 • edited Loading

pangolp commented Sep 15, 2024 •

edited

Loading

Yehonal commented Sep 15, 2024 via email •

edited

Loading

pangolp commented Sep 16, 2024 •

edited

Loading

Helias commented Sep 16, 2024 •

edited

Loading

pangolp commented Sep 16, 2024 •

edited

Loading

Yehonal commented Sep 17, 2024 •

edited

Loading

pangolp commented Sep 17, 2024 •

edited

Loading

pangolp commented Sep 17, 2024 •

edited

Loading

pangolp commented Sep 17, 2024 •

edited

Loading

pangolp commented Sep 21, 2024 •

edited

Loading

pangolp commented Sep 21, 2024 •

edited

Loading