LFG can lock out incorrectly based on miscalculated item levels #11631

Closed
lasershark opened this Issue Feb 20, 2014 · 8 comments

6 participants

@lasershark

This is related to issue: 11095 (especially the recent discussion), but separate enough to give a different issue #.

This issue stems from ItemPrototype.h

float GetItemLevelIncludingQuality() const
can return < 0.

Suggest a failsafe floor of 0, i.e.
return (itemLevel < 0 ? 0 : itemLevel);

Although if it’s really required to take into account Quality, I’d suggest adding to itemLevel in the case of Epic and Legendary, instead of subtracting it from the lower levels as it is currently.

@Aokromes
Member

Update your core before reporting bugs.

@jackpoz
Member
jackpoz commented Feb 22, 2014

do you have How To Reproduce steps for this issue ?

@lasershark

Sorry if my report wasn’t clear. I did read the github source code master to ensure the bug was still present before reporting it, and my core had been recently updated. Since I was reporting a specific error in the logic of a function, probably dating from 2010, I didn’t include any build/db info. I apologize for the breach of protocol. From my server.log of my local fixed version:

Using SSL version: OpenSSL 1.0.1e 11 Feb 2013 (library: OpenSSL 1.0.1e 11 Feb 2013)
Using ACE version: 6.1.4
TrinityCore rev. cb237a4+ 2014-02-13 23:16:49 +0100 (master branch) (Win32, Release) (worldserver-daemon)

I’ll point out the general flaw in the logic of the function, then give a specific example of how this gives an error in the game. Even if the error in the game is masked/fixed elsewhere, given this is the root cause, it should be fixed here as well to prevent future errors.

In the file src/server/game/Entities/Item/ItemPrototype.h, last edited December 2013, there is a flaw in the logic of the function GetItemLevelIncludingQuality().

As a rule, ItemLevel should never be < 0. This function assumes it can always subtract 13 from an ItemLevel and still have a number > 0. Clearly this is not true for low level items, such as starting gear.

Current game impact (one example):

If you want to queue for a dungeon, it calculates gear locks based on average ItemLevel when you log in or go up a level. Therefore if have any really low level gear (less than ilevel 13) you’ll have problems.

This behavior has been noted in retired bug report 11095 in the comments dated early February 2014. Since this behavior has nothing to do with the original bug 11095, or its fix, this issue has been entered to address it.

How To Reproduce:

Create a new character.

Level it to 15.

Try to queue for a dungeon.

They are all locked out based on the gear level being too low. However there are no gear level requirements for low level dungeons.

Fix analysis:

While it is possible to fix this error in the (currently only) caller of the function (player.cpp float Player::GetAverageItemLevel()), better form would be to fix it in GetItemLevelIncludingQuality() itself.

GetItemLevelIncludingQuality() is currently called when figuring out dungeon locks and when figuring out vehicle scaling, both of which are based on the average itemLevel of your gear. I’m not educated enough as to the reasons why it needs to take into account quality as a separate measure rather than just stick with itemLevel. Ideally, we could just drop this function as the impact seems minor.

But assuming it is required then -

Ideally we’d add to the ItemLevel rather than subtract from it. However, this would be a much bigger impact to the existing code (specifically vehicle scaling) and the db values for minimum gear level. Someone with more experience than me would need to make the call if this is worth it.

So for quick fix, minimal impact, and dealing with the root cause I propose putting in a hard lower limit on ItemValue. My suggestion in the original bug report is a hard limit of 0 by changing the return statement to be:

    return (itemLevel < 0 ? 0 : itemLevel);

which fixes the immediate issue. However I can easily make a case for setting the lower limit to 1 instead as it could avoid common issues with 0 values in the future, so:

    return (itemLevel < 1 ? 1 : itemLevel);

However as I’ve never contributed to this project, I leave it up to those more experienced as to if/how to proceed.

@jackpoz
Member
jackpoz commented Feb 27, 2014

I created a character, level'd to level 15, queued for a dungeon and it didn't throw any error
wowscrnshot_022714_201722

Still can't reproduce the issue, please post some item ids to reproduce it

@lasershark

Ok so recent commit c223b88 has masked the issue for a single special case in the LFG browser.

In LFGMgr.cpp, specifically:

if (ar->item_level && player->GetAverageItemLevel() < ar->item_level)

Deals with the case where the required average item level in the access_requirement table is set specifically to 0. Interestingly the commit comment says that averageItemLevel < 0 is impossible, but I’ll always applaud defensive coding.

The underlying issue remains Player::GetAverageItemLevel() and GetItemLevelIncludingQuality() can both unexpectedly return negative values, and I’m having real doubts about the utility of including Quality as a separate metric. I think quality is already just rolled into the itemLevel (i.e. items of better quality have larger itemLevels, with the Blizz-specified exception of Heirlooms).

Since the issue is now masked, to get it to easily reproduce one could change the item_level of ‘Deadmines’ in the access_requirements table to 1. Reload the table (or restart the server), and with starting gear, you’ll end up with a negative iLevel and a lock.

This also impacts the calculation for higher level dungeons, with real iLevel requirements. For example, heroic level 80 dungeons have a minimum ilevel of 180, but if you equip level 180 items of less than Epic or Legendary quality, they will have a -13 impact each to your overall gear level score. Is this expected/desired?

If the powers-that-be determine this is all expected behavior, then I would at least propose putting comments in the functions to that effect. I’ve seen messages that state the expectation that iLevel is always > 0, and right now that is clearly wrong.

@Shauren
Member
Shauren commented Feb 28, 2014

Honestly, I have no idea where does this halfassed calculation "including quality" come from...

@Aokromes Aokromes added the Comp-Core label Mar 26, 2014
@w1sht0l1v3
Contributor

Is this still an issue after eb711fd ?

@joschiwald
Member

yes, because GetAverageItemLevel can return negative values

@Subv Subv closed this in ec4b8da Jun 25, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment