Areatrigger template #18035

Traesh · 2016-10-03T16:43:32Z

Changes proposed:

Basic AreaTrigger Entities handlers.
Currently handle non-moving, non-scaling AreaTrigger created by spell effect 179
handle type SPHERE, BOX, CYLINDER, POLYGON
Added C++ script hooks

Target branch(es): 6.x

Issues addressed:

Related to 6.2.3 Warlord of Draenor. Need support SAI and C++ hook for created areatrigger. #16744
Related to [6.x][Death Knight] Core/Spells: Defile talent #16937

Tests performed: (Does it build, tested in-game, etc)
Builded
Tested ingame with spell 187651 (AreaTrigger 4424), type Cylinder
Tested ingame with spell 163559 (AreaTrigger 2472), type Polygon

Known issues and ToDo list:

This is the basic system, ToDo won't be fixed in this PR :
~~SAI Events~~
Moving areatriggers (Spline)
Scaling areatriggers
Find UNK Flags
Implement unhandled flags

(Re-Opening PR due to a fail on previous repo, all old reviews have been fixed)

ghost · 2016-10-03T20:00:24Z

src/server/game/Entities/AreaTrigger/AreaTrigger.cpp

+
+    switch (GetTemplate()->GetType())
+    {
+        case AREATRIGGER_TYPE_SPHERE:


/home/travis/build/TrinityCore/TrinityCore/src/server/game/Entities/AreaTrigger/AreaTrigger.cpp:155:13: fatal error: enumeration value 'AREATRIGGER_TYPE_NONE' not handled in switch [-Wswitch] switch (GetTemplate()->GetType()) ^

Maybe you could add just the 2 lines

case AREATRIGGER_TYPE_NONE: break;

on top of the existing switches?

missing default added :)

Krudor · 2016-10-14T23:17:24Z

src/server/game/Scripting/ScriptMgr.cpp

@@ -662,6 +666,64 @@ class ScriptRegistrySwapHooks<InstanceMapScript, Base>
    bool swapped;
 };

+/// This hook is responsible for swapping SceneScript's


Does this belong to this PR?

Its a missed change from previous PR (already merged)

Krudor · 2016-10-14T23:18:35Z

Gonna see if I can make time to test this, in any case it's an awesome PR I've personally been looking forward to!

Krudor · 2016-10-15T18:25:04Z

Tested with a 292 vertice polygon along with a script and works like a charm, what a joy to see this working, great job!

Shauren · 2016-10-16T09:31:22Z

src/server/game/Entities/AreaTrigger/AreaTrigger.cpp

    SetObjectScale(1);

    SetGuidValue(AREATRIGGER_CASTER, caster->GetGUID());
    SetUInt32Value(AREATRIGGER_SPELLID, spell->Id);
    SetUInt32Value(AREATRIGGER_SPELLVISUALID, spellVisual);
    SetUInt32Value(AREATRIGGER_DURATION, spell->GetDuration());
+    SetUInt32Value(AREATRIGGER_TIME_TO_TARGET_SCALE, spell->GetDuration()); // Todo


This is the time it takes for the client to play "change scale" animation, you probably should not set it to total duration

It appears to be default behavior though

It has no impact on current implementation (because you send final scale before the areatrigger is visible). You would need to modify scale after it is added to world to notice this

Here is a sniff taked 5 minutes ago on Lei Shen, as you can see by default AREATRIGGER_TIME_TO_TARGET_SCALE should be set to AREATRIGGER_DURATION

http://pastebin.com/jrzzmsUi

Why not parse this field with WPP then and store in db?

Shauren · 2016-10-16T09:32:50Z

src/server/game/Entities/AreaTrigger/AreaTrigger.cpp

+
+    Trinity::AnyUnitInObjectRangeCheck check(this, GetTemplate()->MaxSearchRadius);
+    Trinity::UnitListSearcher<Trinity::AnyUnitInObjectRangeCheck> searcher(this, targetList, check);
+    this->VisitNearbyObject(GetTemplate()->MaxSearchRadius, searcher);


unneeded this-> (in more places, not just here)

robens · 2016-10-18T12:05:28Z

src/server/game/Entities/AreaTrigger/AreaTrigger.cpp

+    //this method uses the ray tracing algorithm to determine if the point is in the polygon
+    int nPoints = vertices.size();
+    int j = -999;
+    int i = -999;


any reason to initialize these (i, j) outside of the 'for' scope?

joschiwald · 2016-10-19T19:38:22Z

src/server/game/Scripting/ScriptMgr.h

+
+};
+
+class TC_GAME_API AreaTriggerEntityScript : public ScriptObject


what about using the existing AreaTriggerScript?

Bad idea imho, there is no reason to add an "OnUpdate" function in AreaTriggerScript by example. This is two totally different things, even if they have close names

I think he ment inherit AreaTriggerScript

Well, as i said, those are two differents things, there is no reason to inherit or use AreaTriggerScript

I agree that this is a bad idea. Keep it separate.

Shauren

Almost good to go.

Shauren · 2016-10-20T07:14:10Z

src/server/database/Database/Implementation/WorldDatabase.cpp

@@ -91,4 +91,6 @@ void WorldDatabaseConnection::DoPrepareStatements()
    PrepareStatement(WORLD_UPD_CREATURE_ZONE_AREA_DATA, "UPDATE creature SET zoneId = ?, areaId = ? WHERE guid = ?", CONNECTION_ASYNC);
    PrepareStatement(WORLD_UPD_GAMEOBJECT_ZONE_AREA_DATA, "UPDATE gameobject SET zoneId = ?, areaId = ? WHERE guid = ?", CONNECTION_ASYNC);
    PrepareStatement(WORLD_SEL_GUILD_REWARDS_REQ_ACHIEVEMENTS, "SELECT AchievementRequired FROM guild_rewards_req_achievements WHERE ItemID = ?", CONNECTION_SYNCH);
+    PrepareStatement(WORLD_SEL_AREATRIGGER_TEMPLATES, "SELECT Id, Flags, MoveCurveId, ScaleCurveId, MorphCurveId, FacingCurveId, Data0, Data1, Data2, Data3, Data4, Data5, ScriptName FROM `areatrigger_template`", CONNECTION_SYNCH);


Please don't use prepared statements for loading templates on startup

Shauren · 2016-10-20T07:15:38Z

src/server/game/Entities/AreaTrigger/AreaTrigger.cpp

    SetObjectScale(1);

    SetGuidValue(AREATRIGGER_CASTER, caster->GetGUID());
    SetUInt32Value(AREATRIGGER_SPELLID, spell->Id);
    SetUInt32Value(AREATRIGGER_SPELLVISUALID, spellVisual);
    SetUInt32Value(AREATRIGGER_DURATION, spell->GetDuration());
+    SetUInt32Value(AREATRIGGER_TIME_TO_TARGET_SCALE, spell->GetDuration()); // Todo


Why not parse this field with WPP then and store in db?

Shauren · 2016-10-20T07:26:47Z

src/server/game/Entities/AreaTrigger/AreaTrigger.cpp

+
+void AreaTrigger::HandleUnitEnterExit(std::list<Unit*> newTargetList)
+{
+    for (Unit* unit : newTargetList)


I believe this would be a better way to handle this:

GuidUnorderedSet exitUnits = _insideUnits; _insideUnits.clear(); for (Unit* unit : newTargetList) { if (exitUnits.erase(unit) == 0) // erase(key_type) returns number of elements erased { // handle entering, chat handler and script hook } _insideUnits.insert(unit->GetGUID()); } for (ObjectGuid const& exitUnitGuid : exitUnits) { // handle exit, no conditions }

Making this O(n) average complexity (unordered_set::erase is O(1) in average case) instead of your propsed O(n^2)
And you don't need to check IsAlive() anywhere, AnyUnitInObjectRangeCheck does that so newTargetList will never contain dead units

I felt like this was the worst part of my PR, Thanks very much :)

Shauren · 2016-10-20T07:27:50Z

src/server/game/Entities/AreaTrigger/AreaTriggerTemplate.h

+#define TRINITYCORE_AREATRIGGER_TEMPLATE_H
+
+#include "Object.h"
+#include "ObjectAccessor.h"


Don't include ObjectAccessor here, you don't need it (do it in .cpp)

Shauren · 2016-10-20T07:30:03Z

src/server/game/Entities/AreaTrigger/AreaTriggerTemplate.cpp

+        }
+        case AREATRIGGER_TYPE_CYLINDER:
+        {
+            MaxSearchRadius = std::sqrt((CylinderDatas.Radius * CylinderDatas.Radius) + (CylinderDatas.Height * CylinderDatas.Height));


VisitNearbyObject expects 2D distance for its first argument, also don't turn this into a sphere if it's a cylinder

Shauren · 2016-10-20T07:36:13Z

src/server/game/Entities/AreaTrigger/AreaTriggerTemplate.cpp

+        }
+        case AREATRIGGER_TYPE_BOX:
+        {
+            MaxSearchRadius = std::max(std::max(BoxDatas.Extents[0], BoxDatas.Extents[1]), BoxDatas.Extents[2]);


Don't take Extents[2] into account here, if the height is large you will make search visit unneccessarily large number of grids.
On top of that the equation should be std::sqrt(BoxDatas.Extents[0] * BoxDatas.Extents[0] / 4 + BoxDatas.Extents[1] * BoxDatas.Extents[1] / 4)

Shauren · 2016-10-20T07:37:49Z

src/server/game/Entities/Object/Object.cpp

-    //    if (HasScaleCurveID)
-    //        *data << uint32(ScaleCurveID);
+        if (hasScaleCurveID)
+            *data << areaTriggerTemplate->ScaleCurveId;


always explicitly specify the type when appending to packets (ObjectGuid is the only exception)
*data << uint32(areaTriggerTemplate->ScaleCurveId);

May i ask why ? Wouldn't it imply to modify code in 2 location if type change ?

And that is a good thing, packet structure should not depend on some local variable having correct type (it also prevents mistakes like using size_t that has different size depending on x86 and x64)

Shauren · 2016-10-20T07:40:01Z

src/server/game/Globals/ObjectMgr.cpp

+
+        areaTriggerTemplate.ScriptId = sObjectMgr->GetScriptId(fields[12].GetCString());
+
+        PreparedStatement* verticesStmt = WorldDatabase.GetPreparedStatement(WORLD_SEL_AREATRIGGER_POLYGON_VERTICES);


Select vertices once at the start and cache them in map<areatriggerId, vector<vertice>>, then just std::move that vector when looping over result from templates

Shauren · 2016-10-20T07:43:54Z

src/server/game/Entities/AreaTrigger/AreaTrigger.cpp

+{
+    std::list<Unit*> targetList;
+
+    Trinity::AnyUnitInObjectRangeCheck check(this, GetTemplate()->MaxSearchRadius);


You will need to extend AnyUnitInObjectRangeCheck constructor to add 3rd argument check3D and pass false to make its inner check IsWithinDistInMap also check only 2D distance here (but it must default to true to keep its current behavior for all other uses in TC)

I updated the box/cylinder/polygon, sphere should check in 3D

Shauren · 2016-10-20T07:45:27Z

src/server/game/Entities/AreaTrigger/AreaTrigger.h

+        std::vector<AreaTriggerPolygonVertice> _polygonVertices;
+
+        AreaTriggerTemplate const* _areaTriggerTemplate;
+        GuidSet _insideUnits;


And use GuidUnorderedSet here

streetrat · 2016-10-20T11:21:39Z

is that normal that you can run thru for example a hunter trap between two update ticks without triggering it if you are fast enough (mounted) ?

Krudor · 2016-10-20T13:35:03Z

I maaay have noticed the same behavior as streetrat described, any thoughts about it Traesh?
It's so responsive on retail, I thought they used a client opcode for sending info whenever a player entered a dynamic areatrigger, much like the static ones, but they don't.

Traesh · 2016-10-20T13:47:46Z

Weird, AreaTrigger update is called at each update there is no delay. Is there a delay between two player coordinates calculation ?

Shauren · 2016-10-20T13:56:04Z

src/server/game/Globals/ObjectMgr.cpp

@@ -9634,9 +9634,33 @@ void ObjectMgr::LoadAreaTriggerTemplates()
 {
    uint32 oldMSTime = getMSTime();
    _areaTriggerTemplateStore.clear();
+    std::map< uint32, std::vector<AreaTriggerPolygonVertice> > verticeByAreatrigger;


dat space padding in template argumets, such stronk c++98

streetrat · 2016-10-20T15:29:59Z

player move hearthbeat is sent from client about once per second if you run straight

joschiwald · 2016-10-20T15:49:27Z

src/server/game/Entities/AreaTrigger/AreaTrigger.cpp

+        {
+            if (Unit* caster = ObjectAccessor::GetUnit(*this, _casterGuid))
+            {
+                caster->AddAura(aura.AuraId, unit);


no spell cast?

It can be discussed, i choose to use AddAura because caster can be dead, or too far away, etc...

Another solution would be to summon a temp trigger with caster faction, just like gameobjects.

some spells are castable while dead, does the spells have limited range that could case too far away issues?

that gameobject cast is bad, don't look on it

AddAura will do nothing for spells with non apply aura spelleffects

Shauren · 2016-10-20T16:09:28Z

@streetrat every half a second to be exact

ghost · 2016-10-20T16:30:29Z

src/server/game/Entities/AreaTrigger/AreaTrigger.cpp

+
+bool AreaTrigger::UnitFitToAuraRequirement(Unit* unit, AreaTriggerAuraTypes targetType) const
+{
+    switch (targetType)


/home/travis/build/TrinityCore/TrinityCore/src/server/game/Entities/AreaTrigger/AreaTrigger.cpp:413:13: fatal error: enumeration value 'AREATRIGGER_AURA_USER_MAX' not handled in switch [-Wswitch] switch (targetType) ^

is this caused by another missing default: break; case switch,
or does it need the actual AREATRIGGER_AURA_USER_MAX ?

Missing default, again :/

OK, I see you have added the default case.
code style suggests that you don't need brackets for 1 single line after default:

ghost · 2016-10-20T18:05:42Z

I know it do not matter at the moment, but nevertheless:
On Future:

struct
{
    uint32 spellId;
    uint32 type;
} removeDynamicObjectOrAreatrigger;

case SMART_ACTION_REMOVE_DYNAMIC_OBJECT_OR_AREATRIGGER:
{
    if (!me)
        break;

    if (e.action.removeDynamicObjectOrAreatrigger.type == 0)
    {
        me->RemoveDynObject(e.action.removeDynamicObjectOrAreatrigger.spellId);
    }

    else if (e.action.removeDynamicObjectOrAreatrigger.type == 1)
    {
        me->RemoveAllDynObjects();
    }

    else if (e.action.removeDynamicObjectOrAreatrigger.type == 2)
    {
        me->RemoveAreaTrigger(e.action.removeDynamicObjectOrAreatrigger.spellId);
    }

    else if (e.action.removeDynamicObjectOrAreatrigger.type == 3)
    {
        me->RemoveAllAreaTriggers();
    }

    else if (e.action.removeDynamicObjectOrAreatrigger.type == 4)
    {
        me->RemoveAllDynObjects();
        me->RemoveAllAreaTriggers();
    }

    else
    {
        TC_LOG_ERROR("sql.sql", "SmartScript: Invalid type for SMART_ACTION_REMOVE_DYNAMIC_OBJECT_OR_AREATRIGGER, skipping");
        break;
    }

    break;
}

Traesh · 2016-10-21T08:04:41Z

If player coordinates are updated each 500 ms i don't think i'll be able to be make areatriggers more reactive than that.

streetrat · 2016-10-21T08:41:50Z

well, g3d library can calculate from 2 different xyz coords of it hits an object or not :)
store the the player position on every update, but before storing check if the last pos + current pos's vector hit the trigger, "simple" :D

Make travis happy

…eatureAI

…ura, improved spline code

…anent areatriggers

Build fix

… movement was in progress

… Spheres

Traesh mentioned this pull request Oct 3, 2016

Core/AreaTrigger : Basic areaTrigger entities system #17997

Closed

5 tasks

ghost reviewed Oct 3, 2016

View reviewed changes

ghost added Feedback-PatchFix Comp-Core Branch-master labels Oct 3, 2016

Aokromes added Feedback-FixOutdatedMissingWIP and removed Feedback-PatchFix labels Oct 4, 2016

Krudor reviewed Oct 14, 2016

View reviewed changes

Shauren reviewed Oct 16, 2016

View reviewed changes

DDuarte changed the base branch from 6.x to master October 16, 2016 16:47

robens reviewed Oct 18, 2016

View reviewed changes

Traesh mentioned this pull request Oct 18, 2016

Add Areatriggers_template to SQL parsing TrinityCore/WowPacketParser#270

Merged

joschiwald reviewed Oct 19, 2016

View reviewed changes

Shauren requested changes Oct 20, 2016

View reviewed changes

Shauren reviewed Oct 20, 2016

View reviewed changes

Traesh force-pushed the areatrigger_template branch from 4ed8bd8 to 5693bb3 Compare October 20, 2016 10:07

Shauren reviewed Oct 20, 2016

View reviewed changes

joschiwald reviewed Oct 20, 2016

View reviewed changes

ghost reviewed Oct 20, 2016

View reviewed changes

Traesh and others added 26 commits January 28, 2017 21:44

Fix reviews

78232b0

Make travis happy

fix reviews

6edb729

Added JustRegisteredAreaTrigger and JustUnregisteredAreaTrigger in Cr…

641ea08

…eatureAI

Fix reviews

717a812

fixed object update values, fixed visual of areatriggers created by a…

86274cc

…ura, improved spline code

where is the save button?

6fd412f

fixed crash, started fixing duration

991692d

Added OnInitialize script hook, updated duration handling, fixes perm…

ec0559f

…anent areatriggers

Core/DataStores: Split AreaTrigger data from ObjectMgr

8bab9bd

Build fix

Added some areatrigger packets

0361ea0

Send ReShape packet on movement path update

2c2158a

Core/AreaTriggers: Fix Position Z calculation

9825f96

Core/AreaTriggers: Fixed ReShape, blizz sends empty reshape packet if…

c45284c

… movement was in progress

Core/AreaTriggers: implemented AREATRIGGER_AURA_USER_CASTER

54f0f4e

Core/AreaTriggers: validate Curves on startup

13f23e8

Core/AreaTriggers: Implemented AREATRIGGER_FLAG_HAS_DYNAMIC_SHAPE for…

85e9353

… Spheres

Core/AreaTriggers: Implemented MoveCurve

fe2e075

Core/AreaTriggers: Added DecalPropertiesId

f369767

Core/AreaTriggers: Splines depend on SpellMisc now

dafadc0

Core/AreaTriggers: Implemented AREATRIGGER_FLAG_HAS_FACE_MOVEMENT_DIR

d474951

Core/AreaTriggers: Added RollPitchYaw to AreaTrigger Class

0a04d6c

some changes here and there, fixes typos

77e5b5a

Core/Transports: Added support for AreaTriggers on transports

3db60c9

fixed initial duration

66ad9f9

prepare merge

00d393c

Renamed areatrigger_template_auras to areatrigger_template_actions

d3b957c

joschiwald force-pushed the areatrigger_template branch from 009c9ce to d3b957c Compare January 28, 2017 22:04

Fixed typo

19487d3

joschiwald merged commit 6ab3877 into TrinityCore:master Jan 28, 2017

Aokromes removed the Feedback-FixOutdatedMissingWIP label Feb 26, 2017


		};

		class TC_GAME_API AreaTriggerEntityScript : public ScriptObject


		areaTriggerTemplate.ScriptId = sObjectMgr->GetScriptId(fields[12].GetCString());

		PreparedStatement* verticesStmt = WorldDatabase.GetPreparedStatement(WORLD_SEL_AREATRIGGER_POLYGON_VERTICES);

Areatrigger template #18035

Areatrigger template #18035

Conversation

Traesh commented Oct 3, 2016 • edited by joschiwald

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

Krudor commented Oct 14, 2016

Krudor commented Oct 15, 2016

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

Shauren Oct 16, 2016 • edited

Choose a reason for hiding this comment

robens Oct 18, 2016 • 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

Shauren left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Shauren Oct 20, 2016 • 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

Shauren Oct 20, 2016 • 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

streetrat commented Oct 20, 2016

Krudor commented Oct 20, 2016

Traesh commented Oct 20, 2016

Shauren Oct 20, 2016 • edited

Choose a reason for hiding this comment

streetrat commented Oct 20, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Shauren commented Oct 20, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Oct 20, 2016 • edited by ghost

Traesh commented Oct 21, 2016

streetrat commented Oct 21, 2016

Traesh commented Oct 3, 2016 •

edited by joschiwald

Shauren Oct 16, 2016 •

edited

robens Oct 18, 2016 •

edited

Shauren Oct 20, 2016 •

edited

Shauren Oct 20, 2016 •

edited

Shauren Oct 20, 2016 •

edited

ghost commented Oct 20, 2016 •

edited by ghost