Core/AreaTrigger : Basic areaTrigger entities system #17997

Traesh · 2016-09-25T02:57:07Z

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

PalWow · 2016-09-25T03:04:46Z

wow.

matukaa · 2016-09-25T06:09:02Z

src/server/game/Scripting/ScriptMgr.h

+
+    public:
+        // Called when the AreaTrigger has just been created
+        void OnCreate(AreaTrigger* areaTrigger) { }


shouldn't all of these be virtual void? sorry if it's a dumb question

Not a dumb question, you were right :)

also, make Travis happy, use/*areaTrigger*/ /*unit*/ and /*diff*/ :D

but all together, amazing job ^^

…into areatrigger_template

Temppus · 2016-09-25T09:18:48Z

src/server/game/Entities/Unit/Unit.h

@@ -2297,6 +2305,9 @@ class TC_GAME_API Unit : public WorldObject
        typedef std::list<GameObject*> GameObjectList;
        GameObjectList m_gameObj;

+        typedef std::list<AreaTrigger*> AreaTriggerList;


vector is prefered i think

Yes, DEFINITELY use a vector

Well Unit::_UnregisterAreaTrigger use the .remove function, which is lacking in std::vector

m_areaTrigger.erase(std::remove(m_areaTrigger.begin(), m_areaTrigger.end(), areaTrigger));

@Shauren erasing twice will cause out of range exception :) either .erase and provide an iterator, or std::remove. std::remove will give you a past-end iterator, so if you call .erase with that, it will crash the server. maybe you wanted std::find() instead of std::remove()?
Also, I suggest making a for through m_areaTriggers on WorldSession::LogoutPlayer, and calling Remove() function for all of them. All other objects get deleted before logging out, except AreaTriggers. this also caused crashes for me (though not on logout, but on login, Map::Update() tried to call _UnregisterAreaTrigger, but it wasn't in m_areaTriggers, so it got invalid itr, and crashed.)

@matukaa std::remove does not remove the element from container. It moves it to the end of it and gives you iterator pointing to it (more precisely, past the end of unerased elements = first erased one)

@Shauren I understand, thanks for clarification :) although m_areaTrigger.erase(std::find()) should also work, no? (completely offtopic)

Shauren · 2016-09-25T09:40:38Z

sql/updates/world/6.x/9999_99_99_17997_world.sql

+  `data5` float NOT NULL DEFAULT '0',
+  `scriptName` char(64) NOT NULL DEFAULT '',
+  PRIMARY KEY (`id`)
+) ENGINE=MyISAM DEFAULT CHARSET=utf8;


#17976 (comment) applies here as well

Wasn't the comment about using MyISAM ?

well, i cant read, ignore it

UpperCamelCase style for columns since all new fields are named so
default uint32 type is int(10) unsigned
add a VerifiedBuild field for tables that have sniffable content

frozen4 · 2016-09-25T11:14:36Z

great work

Naios

Replace self invented techniques by G3D methods.
Add script swap hook
Replace list -> vector

Naios · 2016-09-25T12:26:32Z

sql/updates/world/6.x/9999_99_99_17997_world.sql

@@ -0,0 +1,41 @@
+CREATE TABLE `areatrigger_template` (


Drop the table before creating it please (same in the create statement below)

Naios · 2016-09-25T12:28:52Z

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


    m_valuesCount = AREATRIGGER_END;
    _dynamicValuesCount = AREATRIGGER_DYNAMIC_END;
+
+    _areaTriggerTemplate = nullptr;


Initialize this in L32 after _timeSinceCreated please (_areaTriggerTemplate(nullptr)).

Naios · 2016-09-25T12:30:55Z

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

+            float tempX = vertice.VerticeX;
+            float tempY = vertice.VerticeY;
+
+            vertice.VerticeX = tempX * angleCos - tempY * angleSin;


Is there no rotation implementation you could use from G3D?

I have not found any, but I'm open to suggestions.

Naios · 2016-09-25T12:31:52Z

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

+
+void AreaTrigger::SearchUnitInSphere()
+{
+    std::list<Unit*> targetList;


As Shauren said already: list -> vector

I think UnitListSearcher only accept std::list at this time

Yes you are right, leave it for now as it is, the searchers probably need an overall refactoring...

Naios · 2016-09-25T12:36:16Z

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

+    Trinity::UnitListSearcher<Trinity::AnyUnitInObjectRangeCheck> searcher(this, targetList, check);
+    VisitNearbyObject(GetTemplate()->MaxSearchRadius, searcher);
+
+    float halfExtentsX = extentsX / 2.0;


The implementation of the box seems legit however G3D offers this implementation already through AABox, you could use:

AABox box({x_low, y_low, z_low}, {x_high, y_high, z_high}); ... box.contains({x, y, z});

instead.

Naios · 2016-09-25T12:43:03Z

src/server/game/Scripting/ScriptMgr.cpp

@@ -96,6 +96,10 @@ template<>
 struct is_script_database_bound<AchievementCriteriaScript>
    : std::true_type { };

+template<>
+struct is_script_database_bound<AreaTriggerEntityScript>


AreaTriggerEntityScript also needs a specialization of ScriptRegistrySwapHooks to be swapped correctly when using script hotswapping (otherwise it will cause crashes because unloaded scripts are not cleaned up and will still be referenced).

If you can't make it specialize it as:

template<typename Base> class ScriptRegistrySwapHooks<AreaTriggerEntityScript, Base> : public UnsupportedScriptRegistrySwapHooks<Base> { };

and I will take care of it before it gets merged.

Thanks for review, i will take a look :)

I suppose i have to do the same thing for SceneScript ?

#17976

Yep, good catch. It has to be done for every database bound script (scripts that use scriptnames in the database).

Rochet2 · 2016-09-25T13:26:26Z

Should IDs be used instead to avoid dangling pointers in AreaTriggerList?

# Conflicts: # src/server/game/Accounts/RBAC.h # src/server/game/Globals/ObjectMgr.cpp # src/server/game/Globals/ObjectMgr.h # src/server/game/Scripting/ScriptMgr.cpp # src/server/game/Scripting/ScriptMgr.h # src/server/game/World/World.cpp # src/server/scripts/Commands/cs_reload.cpp

ghost · 2016-10-03T17:51:46Z

Any closing comment (reason for closing) ?

Traesh · 2016-10-03T18:01:40Z

Sry, same reason as Conversation (fail on previous repo), new PR here : #18035

Traesh added 3 commits September 25, 2016 04:27

Implement static AreaTriggers entities

715b7f3

Forgotten rename

06dc199

minor cleanup

d3744a9

ghost added Feedback-PatchFix Comp-Core Branch-master labels Sep 25, 2016

Traesh and others added 3 commits September 25, 2016 05:07

Missing auth RBAC sql file

462b95b

Codingstyle

d88e0b3

Rename 9999_99_99_00_world.sql to 9999_99_99_17997_world.sql

2bddefa

matukaa reviewed Sep 25, 2016

View reviewed changes

Traesh added 2 commits September 25, 2016 10:22

Fix compil & add virtual keyword

9dfac99

Merge branch 'areatrigger_template' of github.com:Traesh/TrinityCore …

c352b46

…into areatrigger_template

Temppus reviewed Sep 25, 2016

View reviewed changes

Shauren reviewed Sep 25, 2016

View reviewed changes

SQL UpperCamelCase & switch AreaTriggerList to std::vector

271e829

Naios requested changes Sep 25, 2016

View reviewed changes

Traesh added 2 commits September 25, 2016 23:26

Fix reviews

38ce9ad

Aokromes added Feedback-FixOutdatedMissingWIP and removed Feedback-PatchFix labels Sep 25, 2016

Traesh closed this Oct 3, 2016

Aokromes removed the Feedback-FixOutdatedMissingWIP label Feb 26, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Core/AreaTrigger : Basic areaTrigger entities system #17997

Core/AreaTrigger : Basic areaTrigger entities system #17997

Traesh commented Sep 25, 2016 •

edited

PalWow commented Sep 25, 2016

matukaa Sep 25, 2016

Traesh Sep 25, 2016

matukaa Sep 25, 2016 •

edited

matukaa Sep 25, 2016

Temppus Sep 25, 2016

Shauren Sep 25, 2016

Traesh Sep 25, 2016

Shauren Sep 25, 2016

matukaa Sep 30, 2016 •

edited

Shauren Sep 30, 2016

matukaa Sep 30, 2016

Shauren Oct 1, 2016

Shauren Sep 25, 2016

Traesh Sep 25, 2016

Shauren Sep 25, 2016

joschiwald Sep 25, 2016

frozen4 commented Sep 25, 2016

Naios left a comment

Naios Sep 25, 2016

Naios Sep 25, 2016 •

edited by Shauren

Naios Sep 25, 2016

Traesh Sep 25, 2016

Naios Sep 25, 2016

Traesh Sep 25, 2016

Naios Sep 25, 2016

Naios Sep 25, 2016

Naios Sep 25, 2016 •

edited

Traesh Sep 25, 2016

Naios Sep 25, 2016 •

edited

Rochet2 commented Sep 25, 2016

ghost commented Oct 3, 2016

Traesh commented Oct 3, 2016 •

edited

Core/AreaTrigger : Basic areaTrigger entities system #17997

Core/AreaTrigger : Basic areaTrigger entities system #17997

Conversation

Traesh commented Sep 25, 2016 • edited

PalWow commented Sep 25, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matukaa Sep 25, 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

matukaa Sep 30, 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

frozen4 commented Sep 25, 2016

Naios left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Naios Sep 25, 2016 • edited by Shauren

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

Naios Sep 25, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Naios Sep 25, 2016 • edited

Choose a reason for hiding this comment

Rochet2 commented Sep 25, 2016

ghost commented Oct 3, 2016

Traesh commented Oct 3, 2016 • edited

Traesh commented Sep 25, 2016 •

edited

matukaa Sep 25, 2016 •

edited

matukaa Sep 30, 2016 •

edited

Naios Sep 25, 2016 •

edited by Shauren

Naios Sep 25, 2016 •

edited

Naios Sep 25, 2016 •

edited

Traesh commented Oct 3, 2016 •

edited