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

fix(CORE/Spells): Add spell range correction for 43458 and 43468 #20294

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

cgrahamseven
Copy link
Contributor

@cgrahamseven cgrahamseven commented Oct 25, 2024

Both the quests The Echo of Ymiron and Anguish of Nifflevar have spells that are meant to be cast for credit at the end of the dialogues. Because their range is set to 0 in Spell.dbc, this does not happen unless you stand on top of the npcs that give credit. This fix adds appropriate ranges to each spell.

Closes AzerothCore issue #19776
Closes AzerothCore issue #18874

Changes Proposed:

This PR proposes changes to:

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

Issues Addressed:

SOURCE:

The changes have been validated through:

  • Live research (checked on live servers, e.g Classic WotLK, Retail, etc.)
  • Sniffs (remember to share them with the open source community!)
  • 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:

  • 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.

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.

Both the quests The Echo of Ymiron and Anguish of Nifflevar have spells
that are meant to be cast for credit at the end of the dialogues.
Because their range is set to 0 in Spell.dbc, this does not happen
unless you stand on top of the npcs that give credit. This fix adds
appropriate ranges to each spell.

Closes AzerothCore issue azerothcore#19776
Closes AzerothCore issue azerothcore#18874
@github-actions github-actions bot added CORE Related to the core file-cpp Used to trigger the matrix build labels Oct 25, 2024
@Nyeriah
Copy link
Member

Nyeriah commented Oct 25, 2024

Sometimes the spells are meant to be cast by the player upon itself and not the creature, so this could use some validation if possible

@cgrahamseven
Copy link
Contributor Author

Sometimes the spells are meant to be cast by the player upon itself and not the creature, so this could use some validation if possible

Ah, so potentially the scripting is wrong here then? How can this be validated?

@Nyeriah
Copy link
Member

Nyeriah commented Oct 25, 2024

That’s a possibility. It can be validated with sniffs

@heyitsbench
Copy link
Contributor

How can this be validated?

Packet sniffs from official servers is usually a surefire way. Currently checking the sniffs I've got from Wrath Classic for this.

@heyitsbench
Copy link
Contributor

43458 is cast by 24315, 43468 is cast by 24321, the latter of which did hit the player with a distance of (3D) 19.520182 (2D) 15.806986 (Exact 3D) 22.020182 (Exact 2D) 18.306986 yards from the caster. AoWoW does show the effect should have a radius of 30y, I suspect maybe that data isn't being used correctly?

@cgrahamseven
Copy link
Contributor Author

43458 is cast by 24315, 43468 is cast by 24321, the latter of which did hit the player with a distance of (3D) 19.520182 (2D) 15.806986 (Exact 3D) 22.020182 (Exact 2D) 18.306986 yards from the caster. AoWoW does show the effect should have a radius of 30y, I suspect maybe that data isn't being used correctly?

Well the scripting is only slightly off since currently 43458 is cast by the ancient male vrykul instead of the female. I don't see that being a huge issue. As far as the spell effects go, Spell.dbc says the effect is 16 - QUEST_COMPLETE and the target a source is TARGET_SRC_CASTER and target b is meant to be TARGET_UNIT_SRC_AREA_ENTRY with a radius of 30 yards. I don't see any real use of TARGET_UNIT_SRC_AREA_ENTRY in SmartAI, it appears to be used some in core scripting though.

@heyitsbench
Copy link
Contributor

Well the scripting is only slightly off since currently 43458 is cast by the ancient male vrykul instead of the female.

For all I can tell, the creature that casts the spell could be either male or female between server restarts, not a big deal at all.

I don't see any real use of TARGET_UNIT_SRC_AREA_ENTRY in SmartAI

If I'm correct, the issue is entirely in the core handling of the QUEST_COMPLETE effect, and not one that can be solved with SmartAI nor should it be solved with SpellInfoCorrections.

@Nyeriah
Copy link
Member

Nyeriah commented Oct 25, 2024

TARGET_UNIT_SRC_AREA_ENTRY require conditions to work, so it targets the creature entry provided (not sure how related that is to this issue )

@cgrahamseven
Copy link
Contributor Author

If I'm correct, the issue is entirely in the core handling of the QUEST_COMPLETE effect, and not one that can be solved with SmartAI nor should it be solved with SpellInfoCorrections.

I just ran another test of the quest and if you cast 43458 on the ancient vrykul male/female it triggers the quest completion and removes you from the spirit world, which seems like the correct behavior. So likely the fix is just to change the scripting to have the ancient vrykul male cast the spell on himself rather than on the action invoker as it currently is.

@Nyeriah
Copy link
Member

Nyeriah commented Oct 25, 2024

Or not. Check my comment above. It needs conditions to hit the correct target.

@heyitsbench
Copy link
Contributor

The sniff does show the spells being cast on the caster, so either way IMO the SmartAI should be adjusted to reflect that, even if that's not the solution for this issue.

@github-actions github-actions bot added DB related to the SQL database and removed CORE Related to the core file-cpp Used to trigger the matrix build labels Oct 25, 2024
@cgrahamseven
Copy link
Contributor Author

The sniff does show the spells being cast on the caster, so either way IMO the SmartAI should be adjusted to reflect that, even if that's not the solution for this issue.

It turns out that only quest 11343 had the action target type set to action invoker instead of self. The other quest associated with this issue had the target type set to self. I tested this change in the smartai and the quest appears to be working correctly now. The other issue about the Anguish of Nifflevar not working can likely be chalked up to the player not being within the 30 yard range of Ymiron and not realizing that. I see plenty of references to that in the comments for the quest on wowhead, so it was clearly meant to be that way in wotlk. Either way, it worked for me as long as I was within range.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DB related to the SQL database Ready to be Reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[QUEST] The Echo of Ymiron & Anguish of Nifflevar Quest The Echo of Ymiron Incompletable
3 participants