Jump to content

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


Guest XTZGZoReX
 Share

Recommended Posts

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.

Link to comment
Share on other sites

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.

Link to comment
Share on other sites

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

Link to comment
Share on other sites

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

Link to comment
Share on other sites

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.

Link to comment
Share on other sites

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.

Link to comment
Share on other sites

 Share

×
×
  • Create New...

Important Information

We have placed cookies on your device to help make this website better. You can adjust your cookie settings, otherwise we'll assume you're okay to continue. Privacy Policy Terms of Use