vmangos / core

Progressive Vanilla Core aimed at all versions from 1.2 to 1.12
GNU General Public License v2.0
691 stars 492 forks source link

🔧 [Support] could you help explain why no break there? // no break #2720

Open william19831103 opened 2 months ago

william19831103 commented 2 months ago

void Item::SaveToDB(bool direct) { uint32 guid = GetGUIDLow(); switch (uState) { case ITEM_CHANGED: { static SqlStatementID updGifts; if (HasFlag(ITEM_FIELD_FLAGS, ITEM_DYNFLAG_WRAPPED)) { SqlStatement stmt = CharacterDatabase.CreateStatement(updGifts, "UPDATE character_gifts SET guid = ? WHERE item_guid = ?"); stmt.PExecute(GetOwnerGuid().GetCounter(), GetGUIDLow()); } } // no break

DeltaF1 commented 2 months ago

I'm going to assume you are familiar with switch statement fall-throughs. If not I recommend looking up that term since it's kind of an unintuitive part of C-style switch statements (or at least I found it unintuitive when I first learned it!) if you aren't already in the headspace of GOTOs and JMP instructions

The code path for ITEM_CHANGED vs ITEM_NEW is mostly the same (Other than using UPDATE vs REPLACE INTO) so letting it fall through into case ITEM_NEW means there's no code duplication. Regardless of whether an item is newly created or already exists in the database, saving it requires collecting all the information about it and sending it to the database so the two cases should be very similar. I'm not sure exactly why the gifts table has to be updated only in the ITEM_CHANGED state, but if you take a look in ItemHandler.cpp here https://github.com/vmangos/core/blob/d58f8f6b00d95655b1d4a8b1acf641a69eeff55a/src/game/Handlers/ItemHandler.cpp#L1231-L1237

you can see a comment that talks about needing to update that table after wrapping. The players inventory is saved to the database (thus calling Item::SaveToDB) as part of wrapping a gift, so depending on the behaviour of SetState I think this just exists for the gift wrapping case.

Addendum on SetState

SetState gets called and then GetState right afterwards. If SetState were just a simple setter then this branch would never be taken since the state was just set to ITEM_CHANGED and so should never be ITEM_NEW. Looking into SetState, if the item is in the NEW state then it doesn't actually change its state to ITEM_CHANGED until later on so this seems to be fine and intended. https://github.com/vmangos/core/blob/d58f8f6b00d95655b1d4a8b1acf641a69eeff55a/src/game/Objects/Item.cpp#L840-L845