Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow nil target for highest target threat data #11

Closed
wants to merge 1 commit into from

Conversation

XScorpion2
Copy link

Solves issue #10

@XScorpion2 XScorpion2 requested a review from dfherr January 18, 2020 14:19
@dfherr
Copy link
Owner

dfherr commented Jan 18, 2020

Hi, thanks for contributing.

Could you walk me through your train of thought why this solves #10? As far as I see this isn't a correct implementation to the specifications of https://wowwiki.fandom.com/wiki/API_UnitThreatSituation

Here are my thoughts so far:

The max nominal threat on any target of a unit is irrelevant. E.g. I could be a damage dealer having dealt 100k threat on the boss and then switch to an add and deal 10k threat, while actually having aggro on that add. That I'm having 100k threat on the securely tanked boss is irrelevant here. Interesting is the 10k nominal but 100% (actively tanking) threat value on the add.

The same problem applies with assuming the target is the boss for calculating the threat status then.

In the situation described above the correct response for the damage dealer would be 3 (for securely tanking at least one mob, namely the add). But the code just assumes the boss and would incorrectly return 0. That's why we have to calculate the threat percentage of every mob for the unit. The threat percentage calculation requires the current target of the mob and this in turn requires a loop over the whole party and hope that anyone is actively targeting the mob right now, which might not always be true).

The only shortcut I can think about is creating a reversed threat table and then assuming threatStatus == 3 (securely actively tanking) on everyone that is highest on the threat table of a particular mob. threatStatus 1 and 2 would be disregarded.

@XScorpion2
Copy link
Author

XScorpion2 commented Jan 18, 2020

Hmm, good point, instead of looking for the highest threat value the unit has against all mobs, I should look for the mob that the unit IS the highest threat. My fault on that logic, that is what I was thinking when trying to implement this and did it wrong.

Definitely needs a mobThreat table as you suggest, will update the logic in GetHighestThreat to do that, the rest of the code should be fine as is.

@dfherr
Copy link
Owner

dfherr commented Jan 18, 2020

If you want to update the merge requests a few pointers I already discussed in regards to #10:

  • Imho the logic should be implemented in UnitThreatSituation and UnitDetailedThreatSituation should not require any changes. If you edit the UnitDetailedThreatSituation like you did, you violate the definiton of https://wowwiki.fandom.com/wiki/API_UnitDetailedThreatSituation
  • I'm not in favor of hacking this, so UnitThreatSituation only decides between threatStatus 0 and 3 and disregards logic for 1 and 2.
  • For efficiency reasons the available party units (party1 part2 or raid1 raid2 raid2pet etc.) should be cached and only updated on changes.
  • For efficiency reasons it might even be necessary to cache the current target of a mob (e.g. if an addon is calling UnitThreatSituation for every party member of the raid for e.g. raid frame updates, you don't want to loop the whole party every single time, as in the worst case this is a redundant 40 consequitive loops, which all yield the same result). So some short term cache (e.g. 0.5seconds) could be used to avoid this.

@XScorpion2 XScorpion2 closed this Nov 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants