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

Scripts/Spells: Warlock Rain of Fire #19345

Merged
merged 7 commits into from Apr 1, 2017

Conversation

Traesh
Copy link
Contributor

@Traesh Traesh commented Mar 22, 2017

Changes proposed:

  • Fix Warlock spell "Rain of Fire" (http://www.wowhead.com/spell=5740). The Warlock can have multiple "Rain of Fire" areatrigger spawned, but a single target will always receive damage only once per tick
  • Minor cleanup (delete 1 old spell, removed from game)

Target branch(es):

  • master

Issues addressed: None

Tests performed: (Does it build, tested in-game, etc.)
Compiled, tested ingame

Known issues and TODO list:

  • None

@@ -1438,9 +1399,49 @@ class spell_warl_unstable_affliction : public SpellScriptLoader
}
};

// 5740 - Demonic Circle: Summon
Copy link
Contributor

@Exodius Exodius Mar 22, 2017

Choose a reason for hiding this comment

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

Shouldn't this line say
// 5740 - Rain of Fire
? 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, bad copy paste x)


for (AreaTrigger* rainOfFireAreaTrigger : rainOfFireAreaTriggers)
{
GuidUnorderedSet insideTargets = rainOfFireAreaTrigger->GetInsideUnits();
Copy link
Member

Choose a reason for hiding this comment

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

const& - dont copy

@@ -0,0 +1,11 @@
DELETE FROM `areatrigger_template` WHERE `Id`=10133;
INSERT INTO `areatrigger_template` (`Id`, `Type`, `Flags`, `Data0`, `Data1`, `Data2`, `Data3`, `Data4`, `Data5`, `ScriptName`, `VerifiedBuild`) VALUES
(10133, 0, 0, 8, 8, 0, 0, 0, 0, "", 23420);
Copy link
Contributor

Choose a reason for hiding this comment

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

we already have this areatrigger_template in db

INSERT INTO `spell_areatrigger` (`SpellMiscId`, `AreaTriggerId`, `MoveCurveId`, `ScaleCurveId`, `MorphCurveId`, `FacingCurveId`, `DecalPropertiesId`, `TimeToTarget`, `TimeToTargetScale`, `VerifiedBuild`) VALUES
(5420, 10133, 0, 0, 0, 0, 0, 0, 7177, 23420); -- SpellId : 5740

DELETE FROM `spell_script_names` WHERE `spell_id`=5740;
Copy link

Choose a reason for hiding this comment

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

Please delete by ScriptName instead of spell_id to remove any existing spell script names using a different spell ID.

@ghost ghost changed the title Script/Spell : Warlock Rain of Fire Scripts/Spells: Warlock Rain of Fire Mar 25, 2017
@ghost ghost added the Sub-Spells label Mar 25, 2017
@@ -2,6 +2,6 @@ DELETE FROM `spell_areatrigger` WHERE (`SpellMiscId`=5420 AND `AreaTriggerId`=10
INSERT INTO `spell_areatrigger` (`SpellMiscId`, `AreaTriggerId`, `MoveCurveId`, `ScaleCurveId`, `MorphCurveId`, `FacingCurveId`, `DecalPropertiesId`, `TimeToTarget`, `TimeToTargetScale`, `VerifiedBuild`) VALUES
(5420, 10133, 0, 0, 0, 0, 0, 0, 7177, 23420); -- SpellId : 5740

DELETE FROM `spell_script_names` WHERE `spell_id`=5740;
DELETE FROM `spell_script_names` WHERE `spell_id`=5740 OR `ScriptName`="spell_warl_rain_of_fire";
Copy link
Member

Choose a reason for hiding this comment

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

We always use ScriptName because 1 id can have multiple scripts linked to him

Copy link

Choose a reason for hiding this comment

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

Yes, that is taken care of already in commit 42591e5. Notice the use of OR as a divider.
Deleting any spell scripts using spell_id 5740 is just an extra precaution. :-)

Copy link
Contributor

Choose a reason for hiding this comment

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

OR is not AND, it should delete only by ScriptName, not by spell_id

Copy link

@ghost ghost Mar 25, 2017

Choose a reason for hiding this comment

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

Well, if you think about the use of AND, it is very selective whereas OR is not.
This variant opens for deleting both for all cases of ScriptName='spell_warl_rain_of_fire'
as well as deleting that special case of spell_id= 5740.


[edit] ... in case anyone had used that ID in a differently named spell.

Choose a reason for hiding this comment

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

@tkrokli you still don't get it..... delete just by scriptname.... what about spell 5740 maybe has another script linked..... for example for a talent???? then you just unlink it too????

Copy link
Member

Choose a reason for hiding this comment

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

Delete only by ScriptName please.

Copy link
Contributor

@Exodius Exodius Mar 27, 2017

Choose a reason for hiding this comment

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

Long story short:

-DELETE FROM `spell_script_names` WHERE `spell_id`=5740 OR `ScriptName`="spell_warl_rain_of_fire";
+DELETE FROM `spell_script_names` WHERE `ScriptName`="spell_warl_rain_of_fire";

I am gonna write some nonsense for those who don't get what @Keader and @ariel- are talking about.

If there is more than 1 script attached to ID 5740, example, "spell_warl_rain_of_fire" and "spell_warl_rain_of_fire_trigger" and you use

DELETE FROM `spell_script_names` WHERE `spell_id`=5740 OR `ScriptName`="spell_warl_rain_of_fire";

query will delete all scripts related to spell_id=5740 AND all scripts related to ScriptName="spell_warl_rain_of_fire".
So, if there is something that does not need to be deleted like "spell_warl_rain_of_fire_trigger"
If spell spell_id=5740 is linked to some other script, it will delete them as well...

If you use

DELETE FROM `spell_script_names` WHERE `spell_id`=5740 AND `ScriptName`="spell_warl_rain_of_fire";

query will delete only spell_id=5740 related to ScriptName="spell_warl_rain_of_fire" but, it is still better to delete only by the ScriptName so...

DELETE FROM `spell_script_names` WHERE `ScriptName`="spell_warl_rain_of_fire";

is the correct way to do it.

@Shauren Shauren merged commit 8ae2374 into TrinityCore:master Apr 1, 2017
Krudor pushed a commit to Krudor/TrinityCore that referenced this pull request Jul 13, 2017
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

8 participants