[10727][optimize] Map system re-engineering

[Ambal]

Hi, everyone

Want to share piece of code which reimplements our existing Map system. It was redesigned to meet following goals:

1) Much better core scalability with mtmaps patch - from now on load balancing for instances greatly improved.

2) Reduce complexity - class MapInstanced is removed from sources, maps are sorted by key [mapid - instanceid].

3) Reduce memory usage - this parameter should improve a bit since we do not create intermediate maps for instances. This also was a result of improved management of GridInfo objects.

4) Code refactoring - ALL geometry obtaining functions from class Map were moved to specialized TerrainInfo class. Maps have a pointer to their own TerrainInfo object and to obtain it either from Map or WorldObject you can use GetTerrain() function.

Thank you for reading and testing

Here is a link to my repository on github, branch "master": https://github.com/Ambal/mangos

[KiriX]

This cool!

But mmaps cool too... Ehhh... =)

[Mordaunt]

Seems promising Ambal. Good work.

Is this patch going to interfere with the mmap redux patch or can this be applied next to yours? Just curious

[faramir118]

we do not create intermediate maps for instances

If GO spawns are added to vmap, they should be spawned per instance, not per map. Not a problem now, but maybe in the future.

Is this patch going to interfere with the mmap redux patch

There are conflicts, but it wouldn't be to hard to merge them.

[Schmoozerd]

faramir, GOs need a very special treatmeant for LoS checks, likely something

!isSpawned || (isDoor && isOpen) || <other exception-gos>

to skip the gos from LoS

[Ambal]

This patch is currently "under construction" - I plan to revert some changes done in Map class by returning geometry querying functions like GetHeight() to it. This will make less conflicts with other patches + will not ruin the current way how do we create and unload grids.

MapInstanced class is lost forever with this patch - no workarounds for this. No placeholders for map objects which need to be shared across instances - if your code relies on such behavior then it is WRONG.

Also note, there is a new class "TerrainInfo" which is created to be a shareholder for ALL types of map geometry information: maps, vmaps and *type here what do you want to share in each grid*.

Update: currently I'm busy and will not submit any further changes in code until this weekend :rolleyes:

[faramir118]

My point is dynamic terrain, and where that information could be stored. The [Not Yet Implemented] features like collision and LoS for doors, ice blocks, platforms that fall, etc should all be handled per instanceId. Should that be stored in the Map class?

edit: I don't mean to hijack the thread with implementation details of other features. I just wondered if these things are related, or could be made easier by this change.

[Ambal]

Faramir118, to explain you how future map system will be organized in terms of sharing terrain data, lets assume we have a mapId = 12 and we create 3 instances of this map { 1, 2, 3}. Maps are stored using the new key called MapID(mapId, instanceId). To share terrain we use TerrainInfo objects each of which correspond to separate mapId. So ALL instances with mapId = 12 will share the SAME TerrainInfo object to load/unload/query geodata:

{12, 1} --|

{12, 2} --|-- TerrainInfo(12) <- this object stores all GridMap objects, VMaps and any other info you need for specified {mapId}

{12, 3} --|

So we have no duplication of geodata for any instances.

[faramir118]

That's what I understood from reading the patch

It semantically enforces what vmaps already do by loading data only for the base map (instanceId 0).

Now, hypothetically say that there is a mmap which has some dynamic state - a door which blocks a path when closed.

The state of the door on map{12, 1} should be independent of the state of the door on map{12, 2}.

Where does the mmap data belong? Not in TerrainInfo, because that would force all instances to share state.

[Schmoozerd]

I would put the base geometry of doors in an instance to the terrain-info, but only use them depending the state of the door in the map-object

with such a construction we would be able to take care of all DB-spawned GOs, and that should be good enough I think (as doors aren't summoned)

[Ambal]

Faramir118, you still have a choice where you want to hold your mmap objects: those you want to have a unique copy for each map - keep them in Map objects. If you want to share some geometry - place them in TerrainInfo.

[Ambal]

Final version of the patch is ready. Added timers to perform global garbage collection of GridMaps on different time for different mapIds - this is to avoid CPU usage peeks.

[Ambal]

Patch updated: fixed some compile errors related to const_iterator usage + MapInstanced.h/.cpp are removed from includes, .vcproj and Makefiles. Have fun

[blueboy]

Hi,

I get the following error during compilation of your latest code from https://github.com/Ambal/mangos with MaNGOS[10721] on a *nix box using gcc.

  CXX    AchievementMgr.o
In file included from ../../../src/game/../framework/Policies/Singleton.h:27:0,
                from ../../../src/game/../shared/MemoryLeaks.h:42,
                from ../../../src/game/../shared/Common.h:66,
                from ../../../src/game/AchievementMgr.cpp:19:
../../../src/game/../framework/Policies/ThreadingModel.h: In instantiation of ACE_Recursive_Thread_Mutex MaNGOS::ClassLevelLockable<MapManager, ACE_Recursive_Thread_Mutex>::si_mtx:
../../../src/game/../framework/Policies/ThreadingModel.h:136:25:   instantiated from MaNGOS::ClassLevelLockable<T, MUTEX>::Lock::Lock() [with T = MapManager, MUTEX = ACE_Recursive_Thread_Mutex]
../../../src/game/../framework/Policies/SingletonImp.h:39:9:   instantiated from static T& MaNGOS::Singleton<T, ThreadingModel, CreatePolicy, LifeTimePolicy>::Instance() [with T = MapManager, ThreadingModel = MaNGOS::ClassLevelLockable<MapManager, ACE_Recursive_Thread_Mutex>, CreatePolicy = MaNGOS::OperatorNew<MapManager>, LifeTimePolicy = MaNGOS::ObjectLifeTime<MapManager>]
../../../src/game/AchievementMgr.cpp:1121:85:   instantiated from here
../../../src/game/../framework/Policies/ThreadingModel.h:152:76: error: MaNGOS::ClassLevelLockable<MapManager, ACE_Recursive_Thread_Mutex>::si_mtx has incomplete type

I notice that your patch does make some changes in ThreadingModel.h.

Hope this helps

[Ambal]

Hi,

I get the following error during compilation of your latest code from https://github.com/Ambal/mangos with MaNGOS[10721] on a *nix box using gcc.

Hi, blueboy. Thank you for reporting. Open your MapManager.h file and make following change

#include "Platform/Define.h"

#include "Policies/Singleton.h"

#include "ace/Recursive_Thread_Mutex.h" <= make this change from ace/Thread_Mutex.h

#include "Map.h"

#include "GridStates.h"

It should help.

[blueboy]

Hi Ambal,

That did the trick, Cheers

only one more issue

  CXX    GridMap.o
../../../src/game/GridMap.cpp: In constructor TerrainInfo::TerrainInfo(uint32):
../../../src/game/GridMap.cpp:634:45: error: urand was not declared in this scope
make[4]: *** [GridMap.o] Error 1

I solved this by adding #include "Util.h" to GridMap.cpp

Thanks for your help

[Ambal]

Blueboy, thanks alot with VC++ compiler we do use precompiled header so such errors are hard to track under Win. I'll update my patch once again :rolleyes:

[blueboy]

Hi Ambal,

I promise this is the last of the compile issues.

CXX    hellfire_peninsula.lo
../../../../src/bindings/ScriptDev2/scripts/outland/hellfire_peninsula.cpp: In member function void 
npc_demoniac_scryerAI::DoSpawnButtress():
../../../../src/bindings/ScriptDev2/scripts/outland/hellfire_peninsula.cpp:266:49: error: class Map has no member 
named GetHeight

Sorry about this but ScriptDev2 doesn't like your patch. I have traced the problem through to the removal of the function GetHeight(), declared in the Map class (Map.h) and the function itself in Map.cpp. Until ScriptDev2 modifies their scripts not to use this, I suggest you put these back.

EDIT: Yes I see what you mean. If you put GetHeight() back the compiler then complains that it can't find GetGrid(), and so forth ..

  CXX    Map.o
../../../src/game/Map.cpp: In member function float Map::GetHeight(float, float, float, bool, float) const:
../../../src/game/Map.cpp:921:49: error: class Map has no member named GetGrid

I'll be patient and wait for ScriptDev2 to change

../../../../src/bindings/ScriptDev2/scripts/outland/hellfire_peninsula.cpp

..in the meantime here is a small patch I created for the above script

diff --git a/src/bindings/ScriptDev2/scripts/outland/hellfire_peninsula.cpp b/src/bindings/ScriptDev2/scripts/outland/hellfire_peninsula.cpp
index 4046105..f8bd6e7 100644
--- a/src/bindings/ScriptDev2/scripts/outland/hellfire_peninsula.cpp
+++ b/src/bindings/ScriptDev2/scripts/outland/hellfire_peninsula.cpp
@@ -263,7 +263,7 @@ struct MANGOS_DLL_DECL npc_demoniac_scryerAI : public ScriptedAI
        float fX, fY;
        m_creature->GetNearPoint2D(fX, fY, 5.0f, fAngle);

-        float fZ_Ground = m_creature->GetMap()->GetHeight(fX, fY, MAX_HEIGHT);
+        float fZ_Ground = m_creature->GetTerrain()->GetHeight(fX, fY, MAX_HEIGHT);

        uint32 uiTime = TIME_TOTAL - (m_uiSpawnButtressTimer * m_uiButtressCount);
        m_creature->SummonCreature(NPC_BUTTRESS, fX, fY, fZ_Ground, m_creature->GetAngle(fX, fY), TEMPSUMMON_TIMED_DESPAWN, uiTime);

the compiler likes it, but someone will have to go to the outlands and see whether the script likes it

UPDATE: O.K they have now removed the offending GetHeight() call in hellfire_peninsula.cpp

ScriptDev2 [1873] Remove one GetHeight() and make less code, doing the same.

So if you are using [1873]+ there is no need to apply the above patch

Hope this helps

[Ambal]

Hi Ambal,

Sorry about this but ScriptDev2 doesn't like your patch. I suggest you put these back.

Yep, I'm aware of ScriptDev2 incompatibility with current patch. I'll speak with their devs about writing a patch for this project since I can't simply return code back and wait for someone to update ScriptDev2 because of a simple rule: why bother yourself if your staff works fine? Stay tuned.

[alexluana]

is it ready for master?

With respect,

Axel

[Ambal]

Ok, another portion of a fix has gone into repo - TerrainInfo class was not exported correctly and SD2 linked with errors. Thanks to zergtmn for spotting the source of this issue. Fixed.

Also, here is a small patch to compile SD2 with current one:

Change from:

float fZ_Ground = m_creature->GetMap()->GetHeight(fX, fY, MAX_HEIGHT); 

to:

float fZ_Ground = m_creature->GetTerrain()->GetHeight(fX, fY, MAX_HEIGHT); 

This is how the code in hellfire_peninsula.cpp should look like. Have fun

Update: since ScriptDev2 team has removed the conflicting code in rev [1873], SD2 should be fully compatible with current patch.

[Ambal]

Ok, since noone posted any complains about this patch, I suppose it is proven to be as suppa-cool-fast-bug-free-and-reliable as stated in the first post So get ready to see in git repo... if you don't have any objections

[Ambal]

Applied in [10727]. Thanks go to me and everyone involved in commenting and testing