Fix(DB/SAI) update quest 6002

[Dr-Arayashiki]

Changes Proposed:

This PR proposes changes to:

• [ ] Core (units, players, creatures, game systems).
• [ ] Scripts (bosses, spell scripts, creature scripts).
• [x] Database (SAI, creatures, etc).

Issues Addressed:

• Closes

SOURCE:

The changes have been validated through:

• [x] Live research (checked on live servers, e.g Classic WotLK, Retail, etc.)
• [x] Sniffs (remember to share them with the open source community!)
• [x] Video evidence, knowledge databases or other public sources (e.g forums, Wowhead, etc.)
• [ ] The changes promoted by this pull request come partially or entirely from another project (cherry-pick). Cherry-picks must be committed using the proper --author tag in order to be accepted, thus crediting the original authors, unless otherwise unable to be found

Tests Performed:

This PR has been:

• [x] Tested in-game by the author.
• [ ] Tested in-game by other community members/someone else other than the author/has been live on production servers.
• [ ] This pull request requires further testing and may have edge cases to be tested.

How to Test the Changes:

• [ ] This pull request can be tested by following the reproduction steps provided in the linked issue
• [ ] This pull request requires further testing. Provide steps to test your changes. If it requires any specific setup e.g multiple players please specify it as well.

1. .go cre 26661
2. .q add 6002
3. see that the text field while the mission is in progress is no longer blank. Quest progression text on NPC was missing
4. Lunaclaw now targets the player without them having to get close, thus preventing the creature from disappearing if the player summons it and remains still.
5. Additionally, the IDs of the mission areas and points of interest have also been corrected.

reference:

• [x] https://www.wowhead.com/wotlk/quest=6002/body-and-heart
• [x] https://wowpedia.fandom.com/wiki/Body_and_Heart#Progress

VIDEO BLIZZARD

• https://github.com/azerothcore/azerothcore-wotlk/assets/122452427/19e19a02-67bd-4419-a1d8-5ea26faf1c94

Known Issues and TODO List:

• [ ]
• [ ]

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.

[Dr-Arayashiki]

@heyitsbench Maybe Blizzard could have changed the number of some flag-related things in the WOTLK version from now on. I can't imagine what it could be, but the sniff helped a lot to understand how it works, so I was able to put the quest in basically the same way as it behaves on Blizzard. You can test if you want, no problem.

[heyitsbench]

The data directly contradicts sniffed data while having a VerifiedBuild value entered. I cannot approve this PR.

[Dr-Arayashiki]

The data directly contradicts sniffed data while having a VerifiedBuild value entered. I cannot approve this PR.

ok, if you or someone else can do this with the same data obtained from this Sniff, I would appreciate it. Until then, I strongly recommend that this PR remains open for anyone who wants to test and see the revisions you've made. Maybe someone knows why the values ​​captured in the sniff are not working and why the SQL quest present in this PR is

[Dr-Arayashiki]

@heyitsbench I understand what you mean now, I hope. So I only kept the data in Verifiedbuild that I saw was actually being detected and working on AC. And yes, Blizzard changed a lot of things, and nowadays you can't just deposit something from Sniff and expect it to work. Several things in AC are not updated in relation to Retail, I just don't know exactly what, but I can give an example and I don't know if I'm correct: The value of the ObjectiveIndex column, which nowadays is 30, was probably 16 in version 12340 Also, the version that all private servers run on WOTLK is 12340, so you can imagine that many things won't work on AC even with sniff. But I understand your point now.

[pangolp]

What is this pull request? Do you need more changes? Evidence?

[Dr-Arayashiki]

What is this pull request? Do you need more changes? Evidence?

Look, this is an important correction, mainly because it's an initial class quest that confuses a lot of people because it has bugs and doesn't indicate where the player should go, but it seems like the guy above wants it that way. Obviously he prefers to leave it as it is rather than accept something that corrects the problem, even if in a not so conventional way, after all customers are different and things may have changed over time and the sniff unfortunately may not be accurate in some things. If you apply the last SQL I updated, you will see that this is exactly what the quest proposes, both when accepting it and when delivering it. So if people don't want to merge SQL, that's fine, but I'm happy for the other people who used SQL and were super satisfied because now they can do the mission without any headaches and exactly as it is at Blizzard. I'm not going to argue, because if the guy there cared about it he would have at least helped fix the SQL he thinks is faulty (the file is open for editing), but if he hasn't commented by today, it obviously just shows “ how much he cares.” And nothing a person says or comments justifies this correction not being accepted, no matter how many “rules” the AC has.

But I'm glad you commented. thanks :)

[heyitsbench]

[...] because if the guy there cared about it he would have at least helped fix the SQL he thinks is faulty

I can't fix something if I don't know what's wrong with it. This isn't an issue I've had time to look into.

the file is open for editing

Only to repository maintainers, which I am not.

[Dr-Arayashiki]

[...] because if the guy there cared about it he would have at least helped fix the SQL he thinks is faulty

I can't fix something if I don't know what's wrong with it. This isn't an issue I've had time to look into.

the file is open for editing

Only to repository maintainers, which I am not.

That's it my son... Regardless of who you are or your occupation here at AC, if you don't know what's wrong with a SQL that I've tested countless times and seen no problems with, then I don't know what you want. be merged. I'm not going to say anything more, leave it as it is or close this pull and throw the correction in the trash, I'm tired of this conversation that leads nowhere. No hard feelings

[pangolp]

The idea is that the code, if useful, can be merged, but I'm not very aware of it, because I haven't really looked at it. I just wanted to know the status of the pull request. We want to add collaborators, not chase them away. The idea is that contributions, whenever possible, are tested, given feedback, and approved.

[Dr-Arayashiki]

The idea is that the code, if useful, can be merged, but I'm not very aware of it, because I haven't really looked at it. I just wanted to know the status of the pull request. We want to add collaborators, not chase them away. The idea is that contributions, whenever possible, are tested, given feedback, and approved.

So, look at the date this pull was posted. There was time for MANY PEOPLE to test it (and it's a very simple thing to do), and I guarantee there would be no problems, but, as you can see, it doesn't seem to generate interest. So what can we do? That is the question.

[heyitsbench]

There was time for MANY PEOPLE to test it

I have no doubts that this PR works and works well, that's not my issue with it. My issue is that it directly contradicts sniffed data, which in my opinion makes it a hackfix. The issue is what's going wrong to make the sniffed data not work, and that's what I don't know how to fix.

[sudlud]

Theres always multiple approaches on how to fix things. Maybe there's a bigger underlying issue if sniffed data is not working.

Also I know how frustrating it is to put work into a fix and get somehow rejected without any actual suggestion on how to fix it.

I'll try to also have a look at this later today, then we'll get this finalized soon hopefully.

[sudlud]

And I agree with @Dr-Arayashiki that we could definitely need more testers, but I think this has always been an issue.

[elthehablo]

People get turned off from contributing due to us rejecting these kinds of PRs. But it's important for those people to realise what we are trying to do here. We have to implement things the right way.

Unless a bug is very critical (server-crashing or easily abusable bug) we won't merge fixes that directly contradict sniffs. So it's best to look into what a right solution would be. Or leave this open till someone else finds out.

[Dr-Arayashiki]

There was time for MANY PEOPLE to test it

I have no doubts that this PR works and works well, that's not my issue with it. My issue is that it directly contradicts sniffed data, which in my opinion makes it a hackfix. The issue is what's going wrong to make the sniffed data not work, and that's what I don't know how to fix.

The problem is that apparently the detected data (apart from the coordinates of the points of interest which are actually correct) doesn't seem to be working, and I heard that for some time the tool was unable to capture these points and other things. From what I saw, in the POI database in the verifybuild column they all have the value 0 for a reason. Apparently they are all correct, but it seems to me that they are considered a kind of Hackfix because Ymir never managed to get all this information accurately for client 1234, or the information he got never worked correctly because things must have changed since that version, and the client that AC uses continued with that database from that point on. This leads me to believe that it ends up being a matter of trial and error (IN THIS CASE). That's why I wanted to share the sniff I took. In addition to having information collected directly from Blizzard's WOTLK client, we noticed that apparently some data does not work as described in the capture. The certainty we have is that IF the new client's data is different from the client that AC uses, I believe we can demand an acceptance standard that is the same as that of the AC client or that at least resolves even if it is not what we expect. I believe there is room for exceptions.

[Dr-Arayashiki]

People get turned off from contributing due to us rejecting these kinds of PRs. But it's important for those people to realise what we are trying to do here. We have to implement things the right way.

Unless a bug is very critical (server-crashing or easily abusable bug) we won't merge fixes that directly contradict sniffs. So it's best to look into what a right solution would be. Or leave this open till someone else finds out.

So I recommend doing what I said above. Leave it open for people to test and try to figure out how to make it work according to what's in Sniff, because I put in exactly the data that's in the sniff and what I got was a map without any points of interest. The sniff is here in PR, people can feel free to download it. But anyway, this "solves" the problem, but leaves an open question: "why isn't the sniff data working and what is already in the database is?". I tried to figure this out and realized I was at a dead end from the moment I saw that all the other POIs had the VerifiedBuild column with a value of 0, plus there was no information about it anywhere.

[sudlud]

Alright so I just tested this on b6ee4f49c with a lvl 80 troll priest.

• quest text is correctly updated
• ~~I have absolutely no clue where Lunaclaw should be~~ teleport to the quest POI: .go ga 13477
• ~~there's no marking on the map where I could find him~~ after clearing cache and restarting client quest POI is showing correctly now
• quest item can be used at the stone to spawn Lunaclaw
• quest can be completed correctly

works as expected imo

~~what am I doing wrong / what is the expected behavior?~~

~~If everything works~~ I'd propose the following:

• the entries that seem to contradict sniffs have no "VerifiedBuild" set, so it's obvious that its not based on sniffed values
• I've experienced myself that e.g. sniffed spawns sometimes do now show the desired values -> in this case it was always better to manually set values that "just work" and leave a note / comment / do not set a VerifiedBuild until someone knows it better / has better sniffs
• also replacing the full entry when updating with sniffed values even if no values besided VerifiedBuild change is no issue to me - I would to the same when updating spawns with sniffed values so that the sql update actually shows all sniffed data values together with setting the VerifiedBuild

So in my opinion ~~once I'm able to actually correctly test this,~~ we should aprove it as it's an improvement of the quest and the sql updates do not claim to be sniffed as no VerifiedBuild is beeing set on those entries.

If someone wants to "dive in" and get it working with the sniffed ObjectiveIndex of 30 instead of 16 - feel free to go. Until then, this is a valuable improvement.

[Dr-Arayashiki]

Alright so I just tested this on b6ee4f4 with a lvl 80 troll priest.

• quest text is correctly updated
• ~I have absolutely no clue where Lunaclaw should be~ teleport to the quest POI: .go ga 13477
• ~there's no marking on the map where I could find him~ after clearing cache and restarting client quest POI is showing correctly now
• quest item can be used at the stone to spawn Lunaclaw
• quest can be completed correctly

works as expected imo

~what am I doing wrong / what is the expected behavior?~

~If everything works~ I'd propose the following:

• the entries that seem to contradict sniffs have no "VerifiedBuild" set, so it's obvious that its not based on sniffed values
• I've experienced myself that e.g. sniffed spawns sometimes do now show the desired values -> in this case it was always better to manually set values that "just work" and leave a note / comment / do not set a VerifiedBuild until someone knows it better / has better sniffs
• also replacing the full entry when updating with sniffed values even if no values besided VerifiedBuild change is no issue to me - I would to the same when updating spawns with sniffed values so that the sql update actually shows all sniffed data values together with setting the VerifiedBuild

So in my opinion ~once I'm able to actually correctly test this,~ we should aprove it as it's an improvement of the quest and the sql updates do not claim to be sniffed as no VerifiedBuild is beeing set on those entries.

If someone wants to "dive in" and get it working with the sniffed ObjectiveIndex of 30 instead of 16 - feel free to go. Until then, this is a valuable improvement.

my problem with this mission was exactly the POI and the absence of texts that should be there, and this not only in mission 6002, but in the previous ones as well. Before, you picked up the mission and it was already considered completed, which wasn't true, as you had to go to The Barrens and do whatever was necessary to actually complete it. So SQL basically fixes all of that. And seriously... a lot of people came to me asking why they couldn't do the mission or that they only managed it by watching a video on YouTube, since the POI was marked on the NPC, and not in the place where the player was supposed to go. I simply said I would sort it out, and well...... "I sorted it out my way." That's what I managed to do.

Thanks @sudlud

[sudlud]

In the end you could also just not touch the value at all that is being the issue right now.

You could just use an update query to set the correct areaIds (from the sniffs) and that's it - as the "16" was already in the DB before this PR.

[Dr-Arayashiki]

In the end you could also just not touch the value at all that is being the issue right now.

You could just use an update query to set the correct areaIds (from the sniffs) and that's it - as the "16" was already in the DB before this PR.

Bro, I'm going to tell you this with all the sincerity in the world... I got lost with this SQL, it's been a long time since I touched the AzerothCore database. But one thing is certain: all it takes is someone with good will to change the value you are proposing and the correction will continue to work without problems. I have the flu and I'm not in the mood for it at the moment, and I think everyone understands, but you see it's a valuable solution and it works the same way it works at Blizzard, so there's no secret. And I apologize now. It's not laziness, I'm just not feeling well.

try this for me if you can, I'm sure you know the importance of this fix as it is a main quest for the Druid class and is REQUIRED if you want to build a Tanker Druid

[elthehablo]

Closed with https://github.com/azerothcore/azerothcore-wotlk/pull/18465