-
-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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(Script/Instance): Rework Scarlet Monastery Ashbringer event partially #15218
fix(Script/Instance): Rework Scarlet Monastery Ashbringer event partially #15218
Conversation
I have some questions, if anybody can help with these I'd be grateful and could still make some more changes based on the answers.
|
I would recommend you take a look at the implementation for vmangos (Here's one commit cleaning it up a bit). It might not answer all of your questions, but hopefully it'll help at least a little bit. Anything you do cherry-pick, make sure to credit properly to the author(s). |
I tested it overall, and it was better than the previous script, but point 8 is a slight difference from the video |
Is this fix still pulling requests still not complete? How many tasks do I have in the fix solution plan. |
PR abandoned by author. if you want to conitnue, reopen or make a new pr |
Hey! Thanks for the response. I understand that the PR has been marked as abandoned due to my inactivity and absence over the past months. My apologies for the lack of communication during this period. The pull request contains numerous updates and improvements to the Ashbringer event, and I believe it's still of value to AC. There might still be minor improvements to be made, that could be made in the scope of this PR - however, I feel it's important that the work put into this so far doesn't get lost or left behind. With that in mind, I'd like to continue contributing to the project and see this pull request through to completion. It already had the "Ready to be Reviewed" and "Waiting to be Tested" labels for a while, but I'm unsure how to proceed from here. Could you please advise? What would be required on my part to get this PR reviewed, tested, and hopefully, merged? |
Also, how would I actually go about reopening this PR? Do I have to create a completely new PR? I don't see any obvious way to do so in the GitHub UI - I guess I don't have the necessary access rights to reopen it? @Kitzunu |
Reopen as requested, thanks for your contributions |
Wow this is a big one. I would be willing to test this but it's not passing the checks. |
The SQL script does not modify the 'broadcast_text' data table, which copies text from the 'broadcast_text' data table to a variable and then from the variable to the creature_text data table. There is a problem with the SQL Inspector's misjudgment |
Also contains some legibility improvements.
The previous biological velocity was 1.2. The new bioid is from the old Naxxramas. The speed of movement is not the same
|
The creature action of 1:18 seconds in the first video. It can be determined that the event should be triggered by the passive aura of the Ashbrant every 5 seconds (ABEffect000 = 28441). At the same time, this aura effect will also cause some neutral creatures to attack the player. I think the trigger event should be using
I believe in known issues. It seems that the effect of group control is also the effect produced by this passive AOE every 5 seconds |
This reverts commit 5822fc7. The behavior of these Scarlet Monastery NPCs seems to be controlled by both the SAI scripts and the C++ script `npc_scarlet_guard`. Using the new scriptmodel makes the behavior of the NPCs only be controlled by `npc_scarlet_guard`, breaking their combat behavior, as that is implemented in the SAI scripts of the NPC.
I had earlier added a 5 second wait to get the event timings to work nicely, but I believe this was needed in the first place because the wrong NPC was used for the event (fixed in an earlier commit). Now that the correct NPC is used, if the 5 second wait is removed, the event timings work out nicely again.
Okay, pushed some changes again. Here's the short of it:
I concur with @fangshun2004 and reverted the changes to
Yeah, the move speed is not the same, but I think this might be what the move speed is supposed to be. I now removed the 5 second wait from Highlord Mograine (I was the one to add the wait in an earlier commit, as a bandaid, because the timings I saw in the videos vs AzerothCore didn't match up) and the event timings more or less work out. Have you measured the time it takes for Highlord Mograine to reach Renault after spawning at the doors? I could do it from the video, and check if now, with the correct NPC and the wait time removed, the event timings match the timings in the video. BTW @fangshun2004 if you don't mind, I could use your "no weapons" code in this PR, and credit you via the commit author field?
Oh cool, thank you. I will apply this change next time I am working on the code and I will credit you :) |
@fangshun2004 I added the contributions as I mentioned in my previous comment, and credited you in the PR description and in the commit "author" field. Please let me know if this is OK by you - if not, I will remove the contributions from this PR. |
Okay, the TODO list is emptied and, for now, I can't think of any other actions for me to take regrading this PR. As far as I can tell, it's ready for review and testing. I will keep an eye on it and try to react/respond in a timely manner. |
Question: Is any of this sniffed? |
No, none of this is sniffed. Everything is based on the original implementation and what I have observed in the videos in the PR description. Could I get my hands on some sniffed data of the event? I could potentially do a follow up PR to this, to tune stuff based on the sniffed data. 👀
…On Mon, Aug 28, 2023 at 22:48, Kitzunu ***@***.***(mailto:On Mon, Aug 28, 2023 at 22:48, Kitzunu <<a href=)> wrote:
Question: Is any of this sniffed?
|
Feel free to contact me on Discord: @heyitsbench. Unfortunately all I have is a second-hand perspective of the event taking place, but it should at least be a bit of help to you. |
I will not block the this PR if the outcome is greatly improved from how it was before. But check with Bench first to see what he got, and then come back to me 👍 |
@feltwitch @Kitzunu I used @heyitsbench to provide sniffing data and modified the timer, the creature coordinates, and the sequence of actions. Now I don't know if I should keep the full time or delete the last two digits of the time. |
@feltwitch Status? |
I'm not sure how long it will take for me to get that functionality implemented in the tool. I also still have yet to take a look at fangshun2004's recent contribution (which is based on the sniff data, I believe). The job is pretty busy this week and I finally upgraded my PC and I can play Armored Core 6 -> I'll probably have time to work on this again this weekend 🥲 Taking all this into consideration, I'm not sure exactly how long the process of utilizing the sniff data is 🤔 It makes me wonder if it would be a good idea to work toward getting this merged, perhaps with incorporating fangshun2004's contribution to this, and treat the utilization of the sniff data as a separate task. Thoughts? If you have any, please share; either way, I'll be returning to this during the weekend and we'll see how far I get. |
Merge Conflicts |
@fangshun2004 Do you want to continue work on this? Seems like the author has abandoned it |
player->GetCreatureListWithEntryInGrid(ScarletList, NPC_SCARLET_MONK, 4000.0f); | ||
player->GetCreatureListWithEntryInGrid(ScarletList, NPC_SCARLET_CHAMPION, 4000.0f); | ||
player->GetCreatureListWithEntryInGrid(ScarletList, NPC_SCARLET_CHAPLAIN, 4000.0f); | ||
player->GetCreatureListWithEntryInGrid(ScarletList, NPC_FAIRBANKS, 4000.0f); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh boy this is a monster
Grid searches are very expensive
4k yards is pretty much the entire map, do we need to search the entire map?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My guess is that some of the monks and mages in the library will also be affected, and I don't know if that's the case in Blizzard servers
I can continue, but I don't know how to merge the modified code into this project, do I open another pull request and then add this pull request and @feltwitch name? |
Yea your best bet is to cherrypick into a new PR |
Try to slice the PR into parts so we can merge it faster, makes everything overall much easier |
Closed in favor of #17558 |
Hi! This ended up being a pretty big PR. If you are gonna take a look at the code, I recommend reviewing the commits individually. These changes make the Scarlet Monastery Ashbringer event very blizzlike, but there are still some differences in eg. the exact timings. However, the differences are small enough that I am satisfied with how the event plays out now.
Changes Proposed:
broadcast_text
) data for NPC texts. There were typos, mistakes and missing texts for NPCs.Issues Addressed:
SOURCE:
Ashbringer event
broadcast_text
.ashbringer event
on YouTube - sort the results byUpload date
and scroll to the bottom to find vanilla-tbc-wotlk videos.NPC on aggro say
Tests Performed:
How to Test the Changes:
These instructions have been more or less how I have tested these changes. They can provide a starting point for how to test this.
Ashbringer event
.additem 22691
.instance unbind all
.tele smcath
NPC on aggro say and Mograine's yell
.instance unbind all
.tele smcath
Known Issues and TODO List:
Highlord Mograine Transform
when targeted. Remove Transform?Add Fairbanks's missing emotes: for most of his gossip, he does 3 emotes per "page" of text displayed to player.Credits & contributions
Contributed by @fangshun2004:
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.