[Threading/Map][8574] Safety in BattleGround objects

[XTZGZoReX]

PATCH: http://paste2.org/p/442584

PATCH v2: http://paste2.org/p/443389

PATCH v3: http://paste2.org/p/443624

What this does: It makes BattleGround objects use the same method of map lookups as Ambal made WorldObject objects do. This way, we can avoid MapManager and HashMapHolder lookups, which is both safer and faster for map multi-threading. Note that I chose to let BattleGround objects still have a map ID, because it's needed in some special cases (this is mostly because we use BattleGround objects as templates, which is bad, but I don't want to rewrite it all ).

When you test this, look for:

• * Do BGs start and end correctly?
  * Do BG creatures/gameobjects spawn/despawn correctly?
  * Do BG flag pickup/drop events work correctly?
  * Do BGs crash (if so, include backtrace!)?
  

Who has been writing this: Me, with help from balrok and kolomati.

Thanks to everyone who tests this, also.

[kolomati]

MaNGOS: 8558

ScriptDev2: 1422

UDB: 382 + http://udbforums.org/index.php?topic=14328

Do BGs start and end correctly?

Tested WSG, started and ended correctly. 10 testers on both sides.

Do BG creatures/gameobjects spawn/despawn correctly?

Everything has spawned and despawned correctly. Flags were properly placed, doors were fine.

Do BG flag pickup/drop events work correctly?

Events work perfectly well.

Do BGs crash (if so, include backtrace!)?

Haven't crashed for me on a "loaded" server.

Regards.

[cyrex]

Same. 7 vs 9 players

[Ambal]

Nice patch, but 2 notes:

1) place ASSERT(m_map) in GetMap() method to check whether you are trying to access NULL pointer - we need strict logic

2) be sure that your m_map is always valid from assigning pointer until BattleGround object destruction - FindMap() was used to protect code from incorrect usage.

3) Maybe it is even better to pass Map object reference to BattleGround constructor and use Map& BattleGround::m_map

[balrok]

1,2) placing an assert should be save, cause map is only needed for gameobject/creature finding and at this point the map must already be loaded.. also it could get rid of many if(map) checks

only at battleground-destructor i think it's possible that the map won't exist cause

i guess a bg also get's destroyed if no-one enqueues for them - or just think about the battleground-templates which just exist in a virtual way

3) the problem is, that the battleground exists before the map

[XTZGZoReX]

3) the problem is, that the battleground exists before the map

Exactly. Our entire BG system is kinda badly designed, and would take ages to rewrite (to make map exist as soon as BG exists). So for now, this is all we can do.

I can try adding the assert, since balrok is right - the map should definitely be valid at the areas it's being used.

EDIT: Oh, and balrok, similar code can be done for outdoor PvP patch if you still maintain it. Currently outdoor PvP patch uses HashMapHolder way too much. I'll poke you on IRC.

EDIT2: Updated patch in original post.

[alein]

Objects are "perm". When some user get speed aura, next player can get too without wait spawn time.

ps: The problem is the same with 30% damage and other.

[XTZGZoReX]

Objects are "perm". When some user get speed aura, next player can get too without wait spawn time.

ps: The problem is the same with 30% damage and other.

I don't get what you mean and/or how it relates . Can you explain better?

[LordJZ]

As I understood, he speaks of gameobjects that give buffs on BGs (GO_TYPE=6, trap) give them even without been spawned in the world.

[alein]

As I understood, he speaks of gameobjects that give buffs on BGs (GO_TYPE=6, trap) give them even without been spawned in the world.

They are just spawned - doesn't remove when some player get buff.

[XTZGZoReX]

This happens both with and without this patch or only with this patch?

[balrok]

also, without this patch - i'm looking at it

[XTZGZoReX]

Fixed in [8563] - not related to this patch . Thanks for reporting, though.

[XTZGZoReX]

New patch posted - fixes shutdown crash. Thanks to balrok for suggestion.

[vagothcpp]

PATCH: http://paste2.org/p/442584

PATCH v2: http://paste2.org/p/443389

PATCH v3: http://paste2.org/p/443624

What this does: It makes BattleGround objects use the same method of map lookups as Ambal made WorldObject objects do. This way, we can avoid MapManager and HashMapHolder lookups, which is both safer and faster for map multi-threading. Note that I chose to let BattleGround objects still have a map ID, because it's needed in some special cases (this is mostly because we use BattleGround objects as templates, which is bad, but I don't want to rewrite it all ).

When you test this, look for:

• * Do BGs start and end correctly?
  * Do BG creatures/gameobjects spawn/despawn correctly?
  * Do BG flag pickup/drop events work correctly?
  * Do BGs crash (if so, include backtrace!)?
  

Who has been writing this: Me, with help from balrok and kolomati.

Thanks to everyone who tests this, also.

+rep

Keep it up.

[XTZGZoReX]

Someone please move this to Accepted .

[Clerigon]

Someone please move this to Accepted .

MaNGOS: the open source MMORPG server > Patches > ... accepted

[Threading/Map][8574] Safety in BattleGround objects

already in the area

[XTZGZoReX]

It was moved after I posted, but no one posted in thread .