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

[6.x] Core/Spells: Fix rogue talent "Death From Above" #17364

Closed
wants to merge 5 commits into from

Conversation

P-Kito
Copy link
Contributor

@P-Kito P-Kito commented Jun 14, 2016

Changes proposed:

Target branch(es): 6x

Tests performed: Tested Ingame https://www.youtube.com/watch?v=jmPTmxH01Uc&feature=youtu.be

Known issues and TODO list:

  • Overall code improvements
  • CastSpell() needs some parameter to make a spell cast with a certain amount of combopoints

Combopoints are currently saved on first spell use and then cleared & reapplied on last spell use (HACKY)

Please beware, that this code is maybe not final - So stop nasty nit picking and start contributing to this base code for Death from Above.

@P-Kito
Copy link
Contributor Author

P-Kito commented Jun 14, 2016

Before you start nitpicking:
SetCanFly(true) is used -> Yes we need a workaround: If this is not set, the "fly" animation will stutter.
All other bad code: Make sure to provide a better solution and help me fix it, find a better solution.

This talent is by far a more complicated one. And I basically provided a good "base". Love <3

@@ -57,11 +57,268 @@ enum RogueSpells
SPELL_ROGUE_HONOR_AMONG_THIEVES = 51698,
SPELL_ROGUE_HONOR_AMONG_THIEVES_PROC = 51699,
SPELL_ROGUE_T5_2P_SET_BONUS = 37169
Copy link

Choose a reason for hiding this comment

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

/home/travis/build/TrinityCore/TrinityCore/src/server/scripts/Spells/spell_rogue.cpp:59:60: fatal error: 
      missing ',' between enumerators
    SPELL_ROGUE_T5_2P_SET_BONUS                     = 37169
                                                           ^
                                                           ,

};

// 163786 - Death from above
#define DFA_DmgModScriptName "spell_rog_death_from_above_dmg_aura"
Copy link
Contributor

Choose a reason for hiding this comment

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

don't use define, it looks ugly, just put that script name where it's needed

{
PrepareSpellScript(spell_rog_death_from_above_jump_target_SpellScript);

void HandleLaunch(SpellEffIndex effIndex)
Copy link

Choose a reason for hiding this comment

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

/home/travis/build/TrinityCore/TrinityCore/src/server/scripts/Spells/spell_rogue.cpp:251:41: fatal error: 
      unused parameter 'effIndex' [-Wunused-parameter]
        void HandleLaunch(SpellEffIndex effIndex)
                                        ^

@P-Kito
Copy link
Contributor Author

P-Kito commented Jun 14, 2016

Note: This talent NEED this PR to be applied too, otherwise the eviscerate/envenom damage will tell you "need to be facing ur target", when the second jump is too far behind the target.
#16885

{
if (spell_rog_death_from_above_AuraScript* script = dynamic_cast<spell_rog_death_from_above_AuraScript*>(aura->GetScriptByName("spell_rog_death_from_above")))
script->SetTarget(GetExplTargetUnit()->GetGUID());
}
Copy link

Choose a reason for hiding this comment

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

braces not needed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's right, but in this case I added it for some overview.

@Aokromes
Copy link
Member

@Shauren this can be merged?

@P-Kito
Copy link
Contributor Author

P-Kito commented Jul 16, 2016

this cannot be merged, the code is really messy - It needs major cleanup/improvements. I gave a solid base... made from sniffs.

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

6 participants