mapcolors: rewrite module implementation

[Artur91425]

Completely rewrote the implementation of the module.

• Fixed a bug with incorrect class colors determination.
• Added coloring of nicknames when mouseover on frames. This feature can be disabled in the settings.
• The new implementation does not use OnUpdate all the time.

90% of the time the game is played, there is no need to update this data... Therefore, in the new implementation, the data is updated only when it is really necessary. Data updating also works on OnUpdate, but now only when the WorldMapFrame and/or BattlefieldMinimap are displayed.

I tested and debugged everything for several days. There shouldn't be any problems

[shikulja]

This makes sense; in the current state, the class colors on the map are often mixed up https://youtu.be/kOfugzU3NMw

[shagu]

Sorry for the late reply. I have been busy and testing this PR requires me to queue to battle grounds and actually play. Which is why I have postponed it all the time till I forgot about it.

First of all: I do like the the code and it's great to see a fix for the broken colors here.

However in my tests the average execution time per update went from (old) 0.0072s to 0.026s.

Old:

New:

Of course, my tests aren't perfect and it might behave different on different machines. But having a factor 10 less performance is in general a sign that soon people will get performance issues with that.

So I don't want to enforce it to everyone as it is right now. Maybe caching the last applied color per icon to not overwrite it each frame, would already be enough? I didn't looked into solutions to it yet.

[Artur91425]

Maybe caching the last applied color per icon to not overwrite it each frame, would already be enough?

No. This won't work. The problem is that the buttons WorldMapParty[1-4], WorldMapRaid[1-40], BattlefieldMinimapParty[1-4], BattlefieldMinimapRaid[1-40] have their unitID changing and therefore the color and texture need to be updated. Therefore, all this works on OnUpdate even in the original code. I know for sure that such an update occurs if you switch map levels in the WorldMapFrame. In addition to this, there were other cases of unitID updating and they cannot be tracked, so there is no point in filling the cache. We'll just break the color match to the player's class.

[Artur91425]

When checking, did you try to limit the number of updates to 1 second or less? if ( this.tick or 1) > GetTime() then return else this.tick = GetTime() + 1 end

It seems to me that even if you make a delay of 1-2 seconds, this will not become a problem. It will still update very quickly and almost imperceptibly, but performance should improve greatly.

[shagu]

Maybe caching the last applied color per icon to not overwrite it each frame, would already be enough?

No. This won't work. The problem is that the buttons WorldMapParty[1-4], WorldMapRaid[1-40], BattlefieldMinimapParty[1-4], BattlefieldMinimapRaid[1-40] have their unitID changing and therefore the color and texture need to be updated. Therefore, all this works on OnUpdate even in the original code. I know for sure that such an update occurs if you switch map levels in the WorldMapFrame. In addition to this, there were other cases of unitID updating and they cannot be tracked, so there is no point in filling the cache. We'll just break the color match to the player's class.

I'm not speaking about caching the class color per unit, but saving the last-set-color-value from SetVertexColor to avoid the SetVertexColor call while r, g, b would be the same as last time. I have noticed on other similar calls that those functions can be hard on performance even if they set the same color as it already had before. So the idea was to only perform SetVertexColor if r ~= last_r, g ~= last_g and b ~= last_b.

[shagu]

When checking, did you try to limit the number of updates to 1 second or less? if ( this.tick or 1) > GetTime() then return else this.tick = GetTime() + 1 end

It seems to me that even if you make a delay of 1-2 seconds, this will not become a problem. It will still update very quickly and almost imperceptibly, but performance should improve greatly.

No, I haven't. I just used the PR as it was. Using a limit like that is something we can do the moment we get out of other ideas :grin:

[Artur91425]

Added caching. It should get better. I also added a delay in calling UpdateUnitFrames and UpdateUnitColors up to 2 times per second. I tried to do it 1 time per second, but it was updated quite slowly and “jumps” in data updating were visible. In my opinion, calling those functions 2 times per second is enough. And now, with added caching, actual data updating occurs very rarely and selectively, only where a data change actually occurred.

P.S. I forgot to remove the debug output before committing...

[shagu]

Thanks! Looking good now in pfDebug :) I have removed the debug output and a no longer required function call via: https://github.com/shagu/pfUI/commit/cecc334e6b9336281a7d74ff453d2aededc3b663