[fixed] m_deleted is never changed?

[zergtmn]

After taking a look at commit http://github.com/mangos/mangos/commit/5003fd95f20752c1ce19b83de0296fedc56ed557 I can't find any place where m_deleted is changed to true.

If I correctly understand its purpose there should be something like http://paste2.org/p/936865

[Schmoozerd]

hm, are you sure this var is still to be used?

void Unit::Update( uint32 p_time )
{
....
   // WARNING! Order of execution here is important, do not change.
   // Spells must be processed with event system BEFORE they go to _UpdateSpells.
   // Or else we may have some SPELL_STATE_FINISHED spells stalled in pointers, that is bad.
   m_Events.Update( p_time );
   _UpdateSpells( p_time );

   CleanupDeletedAuras();
-- -------------  // shouldn't deleted auras be removed at very first?
void Unit::CleanupDeletedAuras()
{
   for (SpellAuraHolderList::const_iterator iter = m_deletedHolders.begin(); iter != m_deletedHolders.end(); ++iter)
       delete *iter;
   m_deletedHolders.clear();

   // really delete auras "deleted" while processing its ApplyModify code
   for(AuraList::const_iterator itr = m_deletedAuras.begin(); itr != m_deletedAuras.end(); ++itr)
       delete *itr;
   m_deletedAuras.clear();
}

I think we should replace IsDeleted with

IsDeleted return m_deletedAuras.find(this) != m_deletedAuras.end();

[zergtmn]

Searching for element in list has complexity of O(n). It's slow.

[Schmoozerd]

Searching for element in list has complexity of O(n). It's slow.

Shouldn't be a list of pointers be sorted, and hence as a better searching (like O(log n))?

Well ok, ram:

a player won't have in average more than 100 auras, there are 200 players online, so the additional bool (I think by now a byte and not more a bit)

will take 20kb of ram => ok, perhaps better to keep the redundant information (except that every redundancy might yield problems if forgotten at one place)

[Lynx3d]

*cough*

Did you try removing Aura::IsDeleted() and recompile? Notice something? ^^

Shouldn't be a list of pointers be sorted, and hence as a better searching (like O(log n))?

It's only a garbage collection list, so it's not meant to be searched. If you sort it, inserting goes from O(1) to O(log n).

[zergtmn]

bool m_permanent:1;
bool m_isPassive:1;
bool m_isDeathPersist:1;
bool m_isRemovedOnShapeLost:1;
bool m_isSingleTarget:1;
bool m_deleted:1;

All these bools are packed into single byte. So we shouldn't care...

Shouldn't be a list of pointers be sorted, and hence as a better searching (like O(log n))?

No. Even if elements in list are sorted search in list require access to all elements in worst case.

*cough*

Did you try removing Aura::IsDeleted() and recompile? Notice something? ^^

But SpellAuraHolder::IsDeleted() is still used.

[Schmoozerd]

*cough*

Did you try removing Aura::IsDeleted() and recompile? Notice something? ^^

No, I was happy to search for IsDeleted, and as I found points, I suggested to replace IsDeleted with looking into the removal list (to avoid additional problems with redundant data)

But you have style reason "garbage list"

and Zergtmn has very good technical reason to implement using the bool,

and we all would love to see "to be deleted auras" to have IsDeleted() == true

[VladimirMangos]

Before aura can be listed in Unit aura list sometime after deleting because locked and in recursive code possible cases when it need checks in past.

Now single way access aura from unit is holder way (aura mods lists updated at delete directly) and holder remove also pointer to auara at delete.

Possible case when aura deleted from own aura apply/update code in like case instead check deleted flag possible use new fast way access code for holder auras:

aura->GetHolder()->GetAuraByEffect(aura->GetEffIndex())

Main isDeleted functionality moved to holder list that replace aura list.

I will drop field in next commit. Thank you for catch outdated code.

[zergtmn]

Sorry but I'm still unable to find any place using 'Find In Files' in Visual Studio where m_deleted is set to true.

My suggestion: http://paste2.org/p/937517

[VladimirMangos]

As Lynx3d point ot me old Aura::IsDeleted als has been broken from start (i forgot set to true) line in my commit

It still need at holder level because must prevent calls for example spell boosts at apply if aura removed at apply by some reason.

I think your fix correct. I look related code for be sure.

[added]In [10315] thank you

I add IsDeleted() check to SpellAuraHolder::ApplyAuraModifiers for sure that if aura holder вудувеув at some aura apply

next auras in holder not wrongly applied after holder deleting.