Scripts/ScarletEnclave: Grand Theft Palomino (quest 12680) improvements #20897

burbur · 2017-11-17T00:05:10Z

Changes proposed:

Quest 12680 - Grand Theft Palomino corrections:

Quest credit can only be taken if near Salanar.
Player is dismounted after using spell 52264 - Deliver Stolen Horse.
Dark Rider of Acherus does not despawn anymore after the cast of spell 52267 - Despawn Horse.
Dark Rider of Acherus aestetich corrections: now he moves towards the horse, say the text and apply the visual effect to the horse after the player uses 52264 - Deliver Stolen Horse to complete the quest.

Target branch(es):

3.3.5
master

Issues addressed: Closes #20844

Tests performed: (Does it build, tested in-game, etc.)
Does it build, tested-in-game, quest rewarded.

Known issues and TODO list:
No issues found.

Thanks to @tkrokli for the sql formatting.

ghost · 2017-11-17T00:38:43Z

src/server/scripts/EasternKingdoms/ScarletEnclave/chapter1.cpp

@@ -1244,6 +1276,7 @@ void AddSC_the_scarlet_enclave_c1()
    new npc_eye_of_acherus();
    new npc_death_knight_initiate();
    new npc_salanar_the_horseman();
+	new spell_deliver_stolen_horse();


Please replace that <tab> with 4 spaces.

Aokromes · 2017-11-17T01:29:58Z

TrinityCore/src/server/scripts/EasternKingdoms/ScarletEnclave/chapter1.cpp:705:17: fatal error: use of undeclared identifier 'TargetGUID'

            TargetGUID = who->GetGUID();

            ^

ghost · 2017-11-17T08:30:59Z

src/server/scripts/EasternKingdoms/ScarletEnclave/chapter1.cpp

+            return ValidateSpellInfo({ SPELL_DELIVER_STOLEN_HORSE, SPELL_EFFECT_STOLEN_HORSE });
+        }
+
+        void CheckHitSalanar(SpellEffIndex effIndex)


src/server/scripts/EasternKingdoms/ScarletEnclave/chapter1.cpp:806:44: fatal error: unused parameter 'effIndex' [-Wunused-parameter] void CheckHitSalanar(SpellEffIndex effIndex) ^ 1 error generated.

This parameter needs to be commented out:

void CheckHitSalanar(SpellEffIndex /*effIndex*/)

All right, I'll comment it out when I'm back from work this evening!
Hope it's all 8)

Keader · 2017-11-17T18:42:22Z

src/server/scripts/EasternKingdoms/ScarletEnclave/chapter1.cpp

-            uint32 Phase;
-            bool Intro;
-            ObjectGuid TargetGUID;
+            ObjectGuid HorseGUID;


_horseGUID;
_events;

private members need has _ prefix

Ok, just renamed the previous ones, without the underscore :)

Keader · 2017-11-17T18:43:15Z

src/server/scripts/EasternKingdoms/ScarletEnclave/chapter1.cpp

+
+                events.ScheduleEvent(EVENT_START_MOVE, 0);
+                events.ScheduleEvent(EVENT_DESPAWN_HORSE, 5000);
+                events.ScheduleEvent(EVENT_END_SCRIPT, 12000);


use std::chrono , like:
events.ScheduleEvent(EVENT_END_SCRIPT, Seconds(12));

Keader · 2017-11-17T18:44:51Z

src/server/scripts/EasternKingdoms/ScarletEnclave/chapter1.cpp

+        void HitSalanarCheck(SpellEffIndex /*effIndex*/)
+        {
+            // Can only hit Salanar due to TARGET_UNIT_NEARBY_ENTRY
+            if (Unit* salanar = GetHitUnit())


if this spell can hit only units (cant hit objects) GetHitUnit() will be never null in this hook.

Keader · 2017-11-17T18:45:16Z

src/server/scripts/EasternKingdoms/ScarletEnclave/chapter1.cpp

+            if (Player* player = GetHitPlayer())
+                player->RemoveAurasDueToSpell(SPELL_EFFECT_STOLEN_HORSE);
+
+            if (Unit* caster = GetCaster())


SpellScripts caster is never null

Yes i know.. But since in every script there are a lot of checks I used them too. Same for the other.
I can change it anyway!

Keader · 2017-11-17T18:48:44Z

src/server/scripts/EasternKingdoms/ScarletEnclave/chapter1.cpp

+            OnEffectHitTarget += SpellEffectFn(spell_deliver_stolen_horse_SpellScript::HitTargetCheck, EFFECT_1, SPELL_EFFECT_KILL_CREDIT2);
+        }
+
+        bool _salanarNear;


it's not initialized
Use this hook to initialize it: https://github.com/TrinityCore/TrinityCore/blob/3.3.5/src/server/game/Spells/SpellScript.h#L136-L138

joschiwald · 2017-11-19T10:14:10Z

src/server/scripts/EasternKingdoms/ScarletEnclave/chapter1.cpp

+
+        void HitSalanarCheck(SpellEffIndex /*effIndex*/)
+        {
+            // Can only hit Salanar due to TARGET_UNIT_NEARBY_ENTRY


the right way would be to abort spells with nearby targets which don't find any target

It makes sense but I had no idea on how to do it.
Which hook is the best for it?

@joschiwald could you please help?

All right, I've thought that it was already implemented!
Anyway the second part of the spellscript is needed to implement the cosmetic part of the quest.

ghost · 2017-11-22T20:07:24Z

Removed 2 of my own comments together with 3 redundant & outdated ones (already fixed, but not hidden b/c outside of change).

burbur · 2017-11-22T20:24:17Z

Thank you @tkrokli!

Since it's my first pull request, what are the next steps?

ghost · 2017-11-22T21:05:57Z

Mainly make sure you follow up if there are any unanswered or unsolved issues in the PR. If there are issues you can't do anything about (outside the scope of this PR), document it in your Original Post under "Known issues and TODO list". You may want to add something about that "cosmetic part of the quest" which you can't fix yet. Post a comment about your final test results at the current state of the PR.

burbur · 2017-11-22T22:44:09Z

Nono, the cosmetic part is working (I mean the part about the horse calling the Dark Rider of Acherus)!
In the previous comment I told that, even with the patch by @joschiwald, the spell script is needed to call all the functions that are already there.

At this moment the quest is working and it includes the cosmetic final part of the quest, where the Dark Rider of Acherus is moving towards the horse, say his sentence and then cast the whirlwind on the horse, despawning him!

There's nothing more!

ghost · 2017-12-16T10:29:33Z

src/server/scripts/EasternKingdoms/ScarletEnclave/chapter1.cpp

+            return GetCaster()->GetEntry() == NPC_HAVENSHIRE_STALLION || GetCaster()->GetEntry() == NPC_HAVENSHIRE_MARE || GetCaster()->GetEntry() == NPC_HAVENSHIRE_COLT;
+        }
+
+        void HitTargetCheck(SpellEffIndex effIndex)


src/server/scripts/EasternKingdoms/ScarletEnclave/chapter1.cpp:814:43: fatal error: unused parameter 'effIndex' [-Wunused-parameter] void HitTargetCheck(SpellEffIndex effIndex) ^

comment out the unused parameter.

ghost · 2017-12-16T10:29:37Z

src/server/scripts/EasternKingdoms/ScarletEnclave/chapter1.cpp

            }
-
-            void InitDespawnHorse(Unit* who)
+


redundant white space (tabs even). please remove.

ghost · 2017-12-16T10:30:36Z

src/server/scripts/EasternKingdoms/ScarletEnclave/chapter1.cpp

-
-            void InitDespawnHorse(Unit* who)
+			
+			void SpellHitTarget(Unit* target, SpellInfo const* spell) override


tab->spaces

Aokromes · 2017-12-18T09:34:34Z

@burbur can you fix requested changes?

jackpoz · 2017-12-24T11:51:59Z

Build fixed, tabs removed. what's left before we can merge it ?

joschiwald · 2017-12-24T12:46:13Z

src/server/scripts/EasternKingdoms/ScarletEnclave/chapter1.cpp

+
+                if (TempSummon* summon = me->ToTempSummon())
+                    if (Unit* summoner = summon->GetSummoner())
+                        _horseGUID = summoner->GetGUID();


i believe you don't have a summoner on the first reset

joschiwald · 2017-12-24T12:49:42Z

src/server/scripts/EasternKingdoms/ScarletEnclave/chapter1.cpp

+
+        void HitTargetCheck(SpellEffIndex /*effIndex*/)
+        {
+            if (Player* player = GetHitPlayer())


gethitunit not good enough?

joschiwald · 2017-12-24T12:51:15Z

sql/updates/world/3.3.5/9999_99_99_99_world.sql

+(51592, 'spell_gen_despawn_self'),
+(51910, 'spell_gen_despawn_self'),
+(52264, 'spell_deliver_stolen_horse'),
+(54420, 'spell_gen_despawn_self');


don't delete all these spell_gen_despawn_self and readd them again, just delete what is needed

So in this case, an exception can be made from the rule of "only delete entries in spell_script_names by ScriptName " -- so we only delete + insert 52267 spell_gen_despawn_self by spell_id in addition to regular handling of spell_deliver_stolen_horse ?

edit: Never mind, it has been a while since I looked into this and the only point of this was to unlink 52267 and link spell_deliver_stolen_horse. Will fix.

- only perform necessary changes, not redo all the spell IDs

jackpoz · 2017-12-24T14:31:06Z

src/server/scripts/EasternKingdoms/ScarletEnclave/chapter1.cpp

-    SPELL_DESPAWN_HORSE         = 51918
+    SPELL_DESPAWN_HORSE         = 52267,
+
+    EVENT_START_MOVE            = 0,


Never use 0 as Event ID, 0 is used by ExecuteEvent() to mean "no events was executed".
This makes me doubt about the quality of the whole PR. By looking at the code it feels like the EVENT_START_MOVE event is executed over and over and over. Did you debug your code to see what it was doing (with breakpoints and other debugging tools) ?

jackpoz · 2017-12-24T14:31:57Z

@burbur please check the comments in this PR

jackpoz · 2018-01-12T18:34:21Z

src/server/scripts/EasternKingdoms/ScarletEnclave/chapter1.cpp

-                    }
+                    case EVENT_START_MOVE:
+                        me->SetTarget(_horseGUID);
+                        me->SetSpeedRate(MOVE_RUN, 0.4f);


what kind of magic number is 0.4f ?

jackpoz · 2018-01-12T18:37:50Z

Ask here to reopen the PR if anyone wants to work on it

ManuFe added 2 commits November 17, 2017 00:53

Grand Theft Palomino (Quest 12680) corrections.

00788f6

Added newline to sql file.

712d115

ghost changed the title ~~Grand Theft Palomino (Quest 12680) corrections.~~ Scripts/ScarletEnclave: Grand Theft Palomino (quest 12680) improvements Nov 17, 2017

ghost added Branch-3.3.5a Comp-C++Script Feedback-PatchFix Sub-Quests labels Nov 17, 2017

ghost reviewed Nov 17, 2017

View reviewed changes

Aokromes added Feedback-FixOutdatedMissingWIP and removed Feedback-PatchFix labels Nov 17, 2017

Indenting corrections and removed InitDespawnHorse function, not needed.

6a7b4cd

ghost reviewed Nov 17, 2017

View reviewed changes

Commenting unused parameter and functions rename

47b91db

Keader reviewed Nov 17, 2017

View reviewed changes

ManuFe added 3 commits November 17, 2017 23:10

Variables corrections

4b11486

Missing Load return value.

4b99eba

Added more horses to the load check

6688882

joschiwald reviewed Nov 19, 2017

View reviewed changes

ghost deleted a comment from Aokromes Nov 22, 2017

ghost deleted a comment from burbur Nov 22, 2017

Update chapter1.cpp

5ac8e31

ghost reviewed Dec 16, 2017

View reviewed changes

ghost added Feedback-FixOutdatedMissingWIP and removed Feedback-PatchFix labels Dec 18, 2017

jackpoz added 4 commits December 24, 2017 12:22

Merge branch '3.3.5' into GrandTheftPalomino

1f26b1c

Tabs to spaces

5c38335

Fix warning

254ef65

Fix build

ef04c7a

jackpoz added Feedback-PatchFix and removed Feedback-FixOutdatedMissingWIP labels Dec 24, 2017

joschiwald reviewed Dec 24, 2017

View reviewed changes

Update 9999_99_99_99_world.sql

e7104ab

- only perform necessary changes, not redo all the spell IDs

jackpoz requested changes Dec 24, 2017

View reviewed changes

jackpoz added Feedback-FixOutdatedMissingWIP and removed Feedback-PatchFix labels Dec 24, 2017

jackpoz reviewed Jan 12, 2018

View reviewed changes

jackpoz added the Closed-Abandoned label Jan 12, 2018

jackpoz closed this Jan 12, 2018

ghost mentioned this pull request Jan 12, 2018

[3.3.5] Scripts/ScarletEnclave: Grand Theft Palamino (quest 12680) Deliver Stolen Horse (spell 52264) #20844

Closed

Sorikoff mentioned this pull request Jan 19, 2019

Scripts/Scarlet Enclave: Grand Theft Palomino #22967

Merged

2 tasks

Aokromes removed Feedback-FixOutdatedMissingWIP labels Mar 31, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Scripts/ScarletEnclave: Grand Theft Palomino (quest 12680) improvements #20897

Scripts/ScarletEnclave: Grand Theft Palomino (quest 12680) improvements #20897

burbur commented Nov 17, 2017 •

edited

ghost Nov 17, 2017 •

edited by ghost

Aokromes commented Nov 17, 2017

ghost Nov 17, 2017

burbur Nov 17, 2017

Keader Nov 17, 2017

burbur Nov 17, 2017

Keader Nov 17, 2017

Keader Nov 17, 2017 •

edited

Keader Nov 17, 2017

burbur Nov 17, 2017

Keader Nov 17, 2017

joschiwald Nov 19, 2017

burbur Nov 19, 2017

Faq Nov 22, 2017

joschiwald Nov 22, 2017

burbur Nov 22, 2017

ghost commented Nov 22, 2017

burbur commented Nov 22, 2017

ghost commented Nov 22, 2017

burbur commented Nov 22, 2017 •

edited

ghost Dec 16, 2017

ghost Dec 16, 2017 •

edited by ghost

ghost Dec 16, 2017

Aokromes commented Dec 18, 2017

jackpoz commented Dec 24, 2017

joschiwald Dec 24, 2017

joschiwald Dec 24, 2017 •

edited

joschiwald Dec 24, 2017

ghost Dec 24, 2017 •

edited by ghost

jackpoz Dec 24, 2017

jackpoz commented Dec 24, 2017

jackpoz Jan 12, 2018

jackpoz commented Jan 12, 2018


		void InitDespawnHorse(Unit* who)

		void SpellHitTarget(Unit* target, SpellInfo const* spell) override

Scripts/ScarletEnclave: Grand Theft Palomino (quest 12680) improvements #20897

Scripts/ScarletEnclave: Grand Theft Palomino (quest 12680) improvements #20897

Conversation

burbur commented Nov 17, 2017 • edited

ghost Nov 17, 2017 • edited by ghost

Choose a reason for hiding this comment

Aokromes commented Nov 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Keader Nov 17, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Nov 22, 2017

burbur commented Nov 22, 2017

ghost commented Nov 22, 2017

burbur commented Nov 22, 2017 • edited

Choose a reason for hiding this comment

ghost Dec 16, 2017 • edited by ghost

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Aokromes commented Dec 18, 2017

jackpoz commented Dec 24, 2017

Choose a reason for hiding this comment

joschiwald Dec 24, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost Dec 24, 2017 • edited by ghost

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackpoz commented Dec 24, 2017

Choose a reason for hiding this comment

jackpoz commented Jan 12, 2018

burbur commented Nov 17, 2017 •

edited

ghost Nov 17, 2017 •

edited by ghost

Keader Nov 17, 2017 •

edited

burbur commented Nov 22, 2017 •

edited

ghost Dec 16, 2017 •

edited by ghost

joschiwald Dec 24, 2017 •

edited

ghost Dec 24, 2017 •

edited by ghost