azerothcore-wotlk
azerothcore-wotlk copied to clipboard
azerothcore
Throttle movement updates
The motivation behind this PR is to improve performance when having objects set to active. This change is mostly noticable for those who has SetAllCreaturesWithWaypointMovementActive (now SetCreaturesWithLongPathActive) enabled.
The idea behind the SetAllCreaturesWithWaypointMovementActive configuration is to make the world feel more alive by having creatures move around even with no players around so it is always random where the creature are in its path. This makes perfect sense for creatures that travel a very long distance such as Rexxar in Desolace or Time-Lost Proto Drake. However, for creatures that only travels a short distance it doesn't really make any sense imo. Take this creature for example:
Currently we set every creature with waypoints active with the configuration enabled. But I personally think it is lazy and overkill to have creatures with such small path, being set to active as it doesn't matter where it is in its path when players approach it anway. So, the first thing that this PR suggests, is a check that calculates the furthest distance in a creatures path. If the distance is 100 yards (can be discussed) or greater, then we consider it a long path and set it to active, otherwise we skip it.
The second thing that this PR suggests, is throttling random movement updates for creatures. So the way active objects work (when something is set to active like above), is that it keeps itself and every object around it updating even when no players are around. This includes random movement i.e. when creatures with no waypoints just wanders around within a small area, for example boars in Durotar. And this affects performance, because with every movement a lot of calculations is made in terms of terrain, pathing and what not. But does it really make any sense to have creatures wander around randomly when no players are around? No. That's why I added another check which looks for players within the creatures visibility range. If no players are found, we basically just pause the random movement updates.
Changes Proposed:
- Update random movement for creatures only when players are nearby.
- Replace
SetAllCreaturesWithWaypointMovementActiveconfig withSetCreaturesWithLongPathActiveto reduce the number of creatures being set to active.
Issues Addressed:
- None that I know of.
Tests Performed:
This PR has been:
- Compiles.
- Tested in-game.
- Tested in debug mode.
Some quick tests shows a roughly 30% drop in CPU usage and almost 50% drop in update time diff. CPU usage done by random movement updates dropped from ~39% to ~5%, and ~12% to ~8% for waypoint movement updates.
Tests were done with preloading maps enabled and
SetAllCreaturesWithWaypointMovementActive/SetCreaturesWithLongPathActiveenabled and 1 player active. See pictures below.
Before:
After:
Before:
After:
How to Test the Changes:
...
Known Issues and TODO List:
- None that I know of.
How to Test AzerothCore PRs
When a PR is ready to be tested, it will be marked as [WAITING TO BE TESTED].
You can help by testing PRs and writing your feedback here on the PR's page on GitHub. Follow the instructions here:
http://www.azerothcore.org/wiki/How-to-test-a-PR
REMEMBER: when testing a PR that changes something generic (i.e. a part of code that handles more than one specific thing), the tester should not only check that the PR does its job (e.g. fixing spell XXX) but especially check that the PR does not cause any regression (i.e. introducing new bugs).
For example: if a PR fixes spell X by changing a part of code that handles spells X, Y, and Z, we should not only test X, but we should test Y and Z as well.
As for the throttling on random movement updates, I don't know if theres a better or cleaner way to do it. Suggestions are welcome.
As for the throttling on random movement updates, I don't know if theres a better or cleaner way to do it. Suggestions are welcome.
What do you think about adding a field, something like viewersCount, to RandomMovementGenerator<Creature> or to the Creature itself, and managing that counter somewhere inside Player::UpdateVisibilityOf? If this counter equals 0, ignore movement updates.
With this approach, I believe it would lead to performance improvements.
As for the throttling on random movement updates, I don't know if theres a better or cleaner way to do it. Suggestions are welcome.
What do you think about adding a field, something like
viewersCount, toRandomMovementGenerator<Creature>or to theCreatureitself, and managing that counter somewhere insidePlayer::UpdateVisibilityOf? If this counter equals 0, ignore movement updates.With this approach, I believe it would lead to performance improvements.
Hmm, perhaps you are right! I could probably just use m_clientGUIDs which is basically viewersCount.
I will try that later. Thanks for the suggestion 😄
Take into account that m_clientGUIDs belongs to the Player but not Creature. So you probably need to use something else.
True, perhaps it just has to be a boolean assigned to the creature, could be called isVisible or something like that. So along with adding the creature to m_clientGUIDs, isVisible is set to true and when removed again it is set to false.
EDIT: or no you are actually right, it kinda needs to be a list. m_clientGUIDs but for creatures holding players or simply a count.
Anyway, I'll figure it out later 😅 I get the idea.
@walkline I've now changed the way we check if creature is allowed to move with random movement, and it did improve performance slightly compared to my previous solution. It also fixed an issue with my previous solution where far sight wouldn't trigger random movement. Let me know what you think.
To sum it up:
For each map update I go through every object visible in each player's client (m_clientGUIDs) and then check if the object is a creature. If it is, I set _pauseRandomMovement to false, allowing it to move. Inside _setRandomLocation I first check if _pauseRandomMovement is true, and if not I continue and immediately set it to true. That way random movement will only be enabled if the creature is present in a player's client.
I am not sure if there is still a smarter way to do this. I have tried a few other approaches but my issues with them was either the creatures stopped moving regardless of being next to them, or the creatures kept their state (allowed to move) when player went offline.
This solution should work with low numbers, but I have some concerns with larger ones.
Let’s imagine a drastic situation where you have 100 players and 100 creatures in one place, and everyone sees each other.
You fill the "seen" vector with 100 creatures in the first iteration. Then, you need to iterate over 99 more characters, and for every character, iterate over their 100 client GUIDs. Within this iteration, use std::find to search for this GUID in the "seen" vector, which contains 100 records. I believe std::find has O(n) complexity, but in our case, it might be closer to O(n/2).
If my math is correct, for this situation, you would make 100 * 100 * 50 = 500,000 iterations over the "seen" vector. Which sounds like overhead for me.
I like the idea with the "seen" counter because it doesn’t introduce any overhead, but it may be tricky to handle all cases.
I've been trying to think of new ways to solve this issues but haven't been able to come up with anything good yet. It is indeed quite tricky to handle all cases...
what about m_viewerGuids container for objects? it can be filled during player's visibility update work. Then this container can be used here and also in all places where object need to send packets to players that can see it instead of grid search. think of spell cast broadcasting for example
