[3.3.5] Get zone/area IDs from vmap data in the liquid update by Treeston · Pull Request #19840 · TrinityCore/TrinityCore

Treeston · 2017-06-02T17:35:55Z

Right now, every call to GetZoneId/GetAreaId (which seem innocuous enough) actually invokes a full vmaps lookup to find the zone/area id of the object. This is kind of bad.

However, as discussed in #19056, we already do liquid state updates on every server tick for everything, and that's a vmap lookup. And that vmap lookup gives us the zone/area data, we just don't use it.

And even that is kind of stupid, because why are we doing liquid updates every tick if it liquid data is static? Instead, we now do liquid update whenever the WorldObject's position changes.

Then we save it on the WorldObject, and use that info to turn GetZoneId and GetAreaId into simple member accesses.

Treeston · 2017-06-02T17:40:40Z

+        if (area->zone)
+            m_zoneId = area->zone;
+
+    /*if (GetTypeId() == TYPEID_PLAYER)


Heads-up, enable this block if you're reproing to get me some useful info (also, give me a .gps).

how about a timer to be able to know how much it takes?

Shauren · 2017-06-02T17:41:19Z

            virtual bool getAreaInfo(unsigned int pMapId, float x, float y, float &z, uint32 &flags, int32 &adtId, int32 &rootId, int32 &groupId) const=0;
            virtual bool GetLiquidLevel(uint32 pMapId, float x, float y, float z, uint8 ReqLiquidType, float &level, float &floor, uint32 &type) const=0;
+            // get both area + liquid data in a single vmap lookup
+            virtual void getAreaAndLiquidData(unsigned int mapId, float x, float y, float z, uint8 reqLiquidType,


refactor this to output all of these parameters into a structure

Shauren · 2017-06-02T18:15:11Z

+void WorldObject::AddToWorld()
+{
+    Object::AddToWorld();
+    GetMap()->GetZoneAndAreaId(m_zoneId, m_areaId, GetPositionX(), GetPositionY(), GetPositionZ());


Why not GetBaseMap()?

Shauren · 2017-06-02T18:15:42Z

        // transports
        Transport* m_transport;

+        // zone/area ID updated in Unit::Update (only Units can move)


False. GameObjects and DynamicObjects can move when on transport. (and on master AreaTriggers, always)

Hm, should we do the call in their ::Update too? Or are transports always their own area?

Shauren · 2017-06-02T18:17:07Z

-void Unit::UpdateUnderwaterState(Map* m, float x, float y, float z)
+void Unit::UpdatePositionData()
+{
+    PositionFullVMapData data;


FullTerrainStatus (naming - matters here because you are returning GridMap data if on flat/nowmo terrain)

nawuko · 2017-06-02T19:09:01Z

It's somewhat unrealated to this PR, but since you are currently working on it this could an intressting issue to solve: #19761. I did not look into this issue at all, so i'm not sure if zone/area id is even the root cause here.

Treeston · 2017-06-02T19:49:53Z

That'd be an issue with base gridmap/vmap data, @nawuko. Not something this PR will (or aims to) address.

nawuko · 2017-06-02T20:02:36Z

okay, just wanted to be sure its not a problem in the lookup logic. Thanks for looking into that 👍

Treeston · 2017-06-02T20:04:26Z

I honestly can't say whether it is, @nawuko - though it seems unlikely.

Shauren · 2017-06-02T23:09:45Z

+                data.liquidInfo.type = info.hitModel->GetLiquidType();
+                if (!reqLiquidType || (GetLiquidFlagsPtr(data.liquidInfo.type) & reqLiquidType))
+                    if (info.hitInstance->GetLiquidLevel(pos, info, data.liquidInfo.level))
+                        data.hasLiquid = true;


Looks like a good place to use Optional

Would make it inconsistent with other vmap methods though.

use it. (Optional)

Treeston · 2017-06-03T00:45:41Z

@Shauren it is my understanding that any movement of a worldobject while it's added to a map should go through one of the methods calling ->Relocate in Map - is this correct?

Shauren · 2017-06-06T20:37:40Z

+                data.liquidInfo.type = info.hitModel->GetLiquidType();
+                if (!reqLiquidType || (GetLiquidFlagsPtr(data.liquidInfo.type) & reqLiquidType))
+                    if (info.hitInstance->GetLiquidLevel(pos, info, data.liquidInfo.level))
+                        data.hasLiquid = true;


use it. (Optional)

Shauren · 2017-06-06T20:37:55Z


            bool getAreaInfo(unsigned int pMapId, float x, float y, float& z, uint32& flags, int32& adtId, int32& rootId, int32& groupId) const override;
            bool GetLiquidLevel(uint32 pMapId, float x, float y, float z, uint8 reqLiquidType, float& level, float& floor, uint32& type) const override;
+            virtual void getAreaAndLiquidData(unsigned int mapId, float x, float y, float z, uint8 reqLiquidType, AreaAndLiquidData& data) const override;


virtual keyword not needed

Shauren · 2017-06-06T20:38:24Z

    #define VMAP_INVALID_HEIGHT       -100000.0f            // for check
    #define VMAP_INVALID_HEIGHT_VALUE -200000.0f            // real assigned value in unknown height case

+    struct TC_COMMON_API AreaAndLiquidData


*_API macro is only needed when your struct/class has methods implemented in .cpp file

Shauren · 2017-06-06T20:39:32Z

        // transports
        Transport* m_transport;

+        virtual void _ProcessPositionDataChanged(PositionFullTerrainStatus const& data);


don't prefix method names with _

Shauren · 2017-06-06T20:41:40Z

+{
+    WorldObject::_ProcessPositionDataChanged(data);
+
+    /*if (GetTypeId() == TYPEID_PLAYER)


remove debug commented out code

Shauren · 2017-06-06T20:43:27Z


+struct PositionFullTerrainStatus
+{
+    bool hasFullAreaInfo;


pls, Optional (no has*** members)

Shauren · 2017-06-06T20:44:46Z

+
+    // area lookup
+    AreaTableEntry const* areaEntry = nullptr;
+    if (vmapData.hasAreaInfo && !(z + 2.0f > mapHeight && mapHeight > vmapData.floorZ))


dont bring out the negation before (), especially when you have conditions you can easily change (> to <=)

Shauren · 2017-06-06T20:45:31Z

+        data.areaInfo.groupId = vmapData.areaInfo.groupId;
+        data.areaInfo.mogpFlags = vmapData.areaInfo.flags;
+    }
+


Shauren · 2017-06-06T20:46:13Z

    return result;
 }

+void Map::getFullTerrainStatusForPosition(float x, float y, float z, PositionFullTerrainStatus& data, uint8 reqLiquidType) const


method name? make it start with uppercase (you can leave the one in vmapmanager as-is)

Shauren · 2017-06-06T20:46:38Z


        void DisableSpline();

+        void _ProcessPositionDataChanged(PositionFullTerrainStatus const& data) override;


don't prefix method names with _

… liquid info in a single vmap lookup - Use this lookup in Map:: relocation methods to update m_areaId and m_zoneId fields on WorldObject - Adjust GetZoneId/GetAreaId on WorldObject to always return these cached fields - Clean up liquid state handling on Unit and Player since I'm at it (fixes and closes TrinityCore#16489)

Treeston · 2017-06-07T01:33:06Z

Travis build fail is due to outdated boost (it has 1.55, we require 1.59).

Removing in place construction (.emplace, the Boost 1.56 feature we need) would be a pretty significant performance impact, which I'm not overly willing to accept at this point in a core vmap call.

ariel- · 2017-06-07T04:28:46Z

Ref boost::optional move semantics, not the first time this happens:
4c27203#diff-1bfb148491dedaab3a6311bec32f7e7eR172.

Is the performance hit that bad? IIRC POD types were just copied instead moved, you can still use boost::in_place to perform an in-place initialization

Shauren · 2017-06-07T07:52:15Z

We use boost::in_place everywhere in packets on master branch (you just need a no-arg constructor, or just no constructors at all)

Shauren · 2017-06-07T08:04:32Z


+struct PositionFullTerrainStatus
+{
+    typedef VMAP::AreaAndLiquidData::AreaInfo AreaInfo;


I know this is convenient but don't do this. Don't make all users of Map class also include VMapManager

Shauren · 2017-06-07T11:47:15Z

TrinityCore/src/server/game/Handlers/AuthHandler.cpp

Line 36 in c5d3dd9

response.SuccessInfo = boost::in_place();

This is the zero-overhead pattern that is used in master branch for building packets - assigning boost::in_place() no-arg version is the same as calling .emplace() with no args
after that you just assign each field (obviously they cant be const in that case)

…it to update a new field in WorldObject, and use that to feed a new GetFloorZ call on WorldObject.

…TrinityCore#19840)" This reverts commit f6c8497.

…d update (TrinityCore#19840)"" This reverts commit 0bf9e05.

…yCore#19840) * Add new method Map::getFullVMapDataForPosition to get area info and liquid info in a single vmap lookup * Use this lookup in Map:: relocation methods to update m_areaId and m_zoneId fields on WorldObject * Adjust GetZoneId/GetAreaId on WorldObject to always return these cached fields * Clean up liquid state handling on Unit and Player * Hand floor's Z coord up through GetFullTerrainStatusForPosition, use it to update a new field in WorldObject, and use that to feed a new GetFloorZ call on WorldObject. Closes TrinityCore#16489 (cherry picked from commit f6c8497)

Treeston added Branch-3.3.5a Comp-Core Sub-Maps/MMaps/Vmaps labels Jun 2, 2017

Treeston requested review from Shauren and r00ty-tc June 2, 2017 17:35

Treeston changed the title ~~Get zone/area IDs from vmap data in the liquid update~~ [3.3.5] Get zone/area IDs from vmap data in the liquid update Jun 2, 2017

Treeston commented Jun 2, 2017

View reviewed changes

Shauren requested changes Jun 2, 2017

View reviewed changes

Shauren reviewed Jun 2, 2017

View reviewed changes

Treeston mentioned this pull request Jun 3, 2017

[3.3.5] Dynamic Creature/Go spawning #19056

Merged

2 tasks

Treeston requested a review from Shauren June 4, 2017 21:37

Shauren requested changes Jun 6, 2017

View reviewed changes

Treeston force-pushed the 3.3.5-cachezoneareaids branch from 78ced7b to ed93010 Compare June 7, 2017 01:03

Treeston requested a review from Shauren June 7, 2017 01:34

Shauren requested changes Jun 7, 2017

View reviewed changes

yay for overhead

46b3f6c

Treeston requested a review from Shauren June 7, 2017 11:41

Treeston added 2 commits June 7, 2017 21:47

Hand floor's Z coord up through GetFullTerrainStatusForPosition, use …

b462def

…it to update a new field in WorldObject, and use that to feed a new GetFloorZ call on WorldObject.

move GetFloorZ to cpp

521d8df

Shauren approved these changes Jun 7, 2017

View reviewed changes

Shauren merged commit f6c8497 into TrinityCore:3.3.5 Jun 7, 2017

Treeston deleted the 3.3.5-cachezoneareaids branch July 10, 2017 21:02

Jildor added a commit to Jildor/TrinityCore that referenced this pull request Jul 19, 2017

Revert "[3.3.5] Get zone/area IDs from vmap data in the liquid update (…

96f1b25

…TrinityCore#19840)" This reverts commit f6c8497.

Jildor added a commit to Jildor/TrinityCore that referenced this pull request Jul 19, 2017

Revert "[3.3.5] Get zone/area IDs from vmap data in the liquid update (…

4655850

…TrinityCore#19840)" This reverts commit f6c8497.

Jildor added a commit to Jildor/TrinityCore that referenced this pull request Jul 19, 2017

Revert "[3.3.5] Get zone/area IDs from vmap data in the liquid update (…

0bf9e05

…TrinityCore#19840)" This reverts commit f6c8497.

Jildor added a commit to Jildor/TrinityCore that referenced this pull request Jul 22, 2017

Revert "Revert "[3.3.5] Get zone/area IDs from vmap data in the liqui…

bf4df52

…d update (TrinityCore#19840)"" This reverts commit 0bf9e05.

ghost mentioned this pull request Aug 12, 2017

[3.3.5] Fix UpdateMovementFlags ignoring DynamicTree height never Updated into 3.3.5a branch. #20141

Closed


		void DisableSpline();

		void _ProcessPositionDataChanged(PositionFullTerrainStatus const& data) override;

Conversation

Treeston commented Jun 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Shauren Jun 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nawuko commented Jun 2, 2017

Uh oh!

Treeston commented Jun 2, 2017

Uh oh!

nawuko commented Jun 2, 2017

Uh oh!

Treeston commented Jun 2, 2017

Uh oh!

Shauren Jun 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Shauren Jun 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Treeston commented Jun 3, 2017

Uh oh!

Shauren Jun 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Treeston commented Jun 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ariel- commented Jun 7, 2017

Uh oh!

Shauren commented Jun 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Shauren commented Jun 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Treeston commented Jun 2, 2017 •

edited

Loading

Shauren Jun 2, 2017 •

edited

Loading

Shauren Jun 2, 2017 •

edited

Loading

Shauren Jun 6, 2017 •

edited

Loading

Shauren Jun 6, 2017 •

edited

Loading

Treeston commented Jun 7, 2017 •

edited

Loading

Shauren commented Jun 7, 2017 •

edited

Loading

Shauren commented Jun 7, 2017 •

edited

Loading