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

Core/Spells: Improve teleport destination for a certain class of spel… #20434

Merged
merged 2 commits into from
Oct 12, 2017

Conversation

chaodhib
Copy link
Member

Changes proposed:

  • Improve teleport destination for a certain class of spell. The implementation of Spell::SelectImplicitTargetDestTargets to be more specific.

Target branch(es): 3.3.5

Issues addressed: Closes #8758

Tests performed: (Does it build, tested in-game, etc.)
Tested IG. Appears to work well.

Known issues and TODO list:

Nothing that I'm aware of but tests should be made because this change affects a whole class of spells.

dist = objSize + (dist - objSize) * float(rand_norm());
float dist = m_spellInfo->Effects[effIndex].CalcRadius(nullptr);
if (targetType.GetTarget() == TARGET_DEST_TARGET_RANDOM)
dist = dist * float(rand_norm());
Copy link

Choose a reason for hiding this comment

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

dist *= float(rand_norm());

Copy link
Member Author

Choose a reason for hiding this comment

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

changed. thanks.

…l (Killing Spree, Shadowstep, ...)

Close # 8758
@chaodhib
Copy link
Member Author

I analysed Shadowstep. The radius of teleportation in dbc is 2.0y. However, we can see from sniffs that in practise, it's actually 4 yards. A correction is therefore needed. I assume it's an occurrence where the DBC client file does not contain the most updated values used in the server.

@Shauren
Copy link
Member

Shauren commented Sep 27, 2017

Its still 2.0 even in legion - maybe that remaining 2 comes from somewhere else? casters or target combat reach? the mysterious bounding radius?

@chaodhib
Copy link
Member Author

chaodhib commented Sep 27, 2017

I don't think there is another field in the formula other than the spell radius. Because if there is one, the formula should stay valid for every spell of the same category. However, I looked at other spells within the same category (the ones with a spell effect using Spell Target 64 to 71) and so far, the actual distance was just the spell radius. Nothing more. Granted that I need to analyze more spells before feeling confident about that unproven fact.

Also several spells with the name 'Shadowstep' (but not the one used by rogues) also have a 4 yards teleport radius.

All these points points me toward the 'DBC data in client file not up to date' explanation.

PS: I believe Shadowstep is not in Legion anymore. It was replaced by Shadowstrike (?).

PS2: The combat reach for players is 1.5y and their bounding radius is 0.383 (yards?). It doesn't adds up.

@Shauren
Copy link
Member

Shauren commented Sep 28, 2017

No, rogues still have shadowstep (with 2 charges) in legion

As for the range - i have no more ideas

@Shauren Shauren merged commit 9dbef4b into TrinityCore:3.3.5 Oct 12, 2017
@chaodhib
Copy link
Member Author

chaodhib commented Oct 12, 2017

This was not ready to be merged. because
a) Feral Charge - Cat (49376) also needs a correction on the DBC data since the spell effect is missing a radius
b) i acquired a lot of data for this class of the spell and even after a lot of diging, i couldn't figure out the formula used. What seems to be clear is that the formula doesn't use neither the target's nor the caster's combat reach. So I guess that this PR is nonetheless an improvement.

@Shauren
Copy link
Member

Shauren commented Oct 13, 2017

Well, there was no [WIP] tag on it so I assumed it was ready

Shauren pushed a commit that referenced this pull request Dec 31, 2020
…class of spells (Killing Spree, Shadowstep, ...) (#20434)

Close # 8758

(cherry picked from commit 9dbef4b)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants