fix(Core/Grids): Grid improvements #20955

Takenbacon · 2024-12-17T23:00:17Z

!! PR is WIP and not ready to be merged

Dynamically allocates grid cells. At (64 * 64) * (8 * 8) = 262,144 cells per map improving the memory efficiency of the structure has a significant impact considering most cells are usually empty anyways. When compiled in RelWithDebug and using the PreloadAllNonInstancedMapGrids = 1 config option, RAM usage decreased from 9,810MB to 4,645MB (no mmaps) and startup time improved from 19 seconds to 16 seconds.
Greatly simplify ObjectGridLoader

While the changes as of this moment are testable, there is more I'd still like to do, thus PR is WIP for now.

Changes Proposed:

This PR proposes changes to:

Core (units, players, creatures, game systems).
Scripts (bosses, spell scripts, creature scripts).
Database (SAI, creatures, etc).

Issues Addressed:

Closes

SOURCE:

The changes have been validated through:

Live research (checked on live servers, e.g Classic WotLK, Retail, etc.)
Sniffs (remember to share them with the open source community!)
Video evidence, knowledge databases or other public sources (e.g forums, Wowhead, etc.)
The changes promoted by this pull request come partially or entirely from another project (cherry-pick). Cherry-picks must be committed using the proper --author tag in order to be accepted, thus crediting the original authors, unless otherwise unable to be found

Tests Performed:

This PR has been:

Tested in-game by the author.
Tested in-game by other community members/someone else other than the author/has been live on production servers.
This pull request requires further testing and may have edge cases to be tested.

How to Test the Changes:

This pull request can be tested by following the reproduction steps provided in the linked issue
This pull request requires further testing. Provide steps to test your changes. If it requires any specific setup e.g multiple players please specify it as well.

Known Issues and TODO List:

[ ]
[ ]

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.

Grimdhex · 2024-12-18T05:04:23Z

One small change if you want for more readability: go out the GridMap inside the Map file. And put it in this own file.

Takenbacon · 2024-12-18T17:50:46Z

One small change if you want for more readability: go out the GridMap inside the Map file. And put it in this own file.

Yeah I saw that, I'm currently weighing how much I should change in a single PR, and also considering this is specifically marked "grids" whereas some of my changes would be more related to map class.

Kitzunu · 2024-12-18T19:13:39Z

Interesting 👍

Grimdhex · 2024-12-20T16:44:20Z

src/server/game/Maps/Map.cpp

@@ -447,7 +443,7 @@ void Map::EnsureGridCreated_i(const GridCoord& p)
    if (!getNGrid(p.x_coord, p.y_coord))
    {
        // pussywizard: moved setNGrid to the end of the function
-        NGridType* ngt = new NGridType(p.x_coord * MAX_NUMBER_OF_GRIDS + p.y_coord, p.x_coord, p.y_coord);
+        NGridType* ngt = new NGridType(p.y_coord * MAX_NUMBER_OF_GRIDS + p.x_coord, p.x_coord, p.y_coord);


Why reverse the coordinates here?

To synchronize NGrid ID with CoordPair's ID which enables the ObjectGridLoader simplifications. I believe this to have just been a simple mistake in the original grid system implementation as GridId isn't used much let alone in conjunction with a grid CoordPair. Previously, spawned objects were stored by cell ID and the object grid loader would loop all cells in the grid and do a lookup on the _mapObjectGuidsStore map for each cell in the grid. Since we're always loading every cell in a grid anyways, it's easy to cut out the bloat and just store the objects by grid ID instead, simplifying the process.

CoordPair:
[[nodiscard]] uint32 GetId() const { return y_coord * LIMIT + x_coord; }

…ural changes

Grimdhex · 2025-01-21T07:57:39Z

I see that you have moved some grid definitions outside of maps. If you don't these data are duplicated at different places in the framework, and it can be merged. I've a PR that port these changes available only on TC master branch (TrinityCore/TrinityCore@65f82c7#diff-ad66842f3a1bb1684807e622c9f129d330fa7cd3daf4ba9a62254cd2de340c52); It's just for information, not necessary for this PR.

Takenbacon · 2025-01-21T10:42:53Z

I see that you have moved some grid definitions outside of maps. If you don't these data are duplicated at different places in the framework, and it can be merged. I've a PR that port these changes available only on TC master branch (TrinityCore/TrinityCore@65f82c7#diff-ad66842f3a1bb1684807e622c9f129d330fa7cd3daf4ba9a62254cd2de340c52); It's just for information, not necessary for this PR.

Sorry, I don't quite understand. Are you saying you want me to merge in that TC commit?

Grimdhex · 2025-01-21T11:45:44Z

I see that you have moved some grid definitions outside of maps. If you don't these data are duplicated at different places in the framework, and it can be merged. I've a PR that port these changes available only on TC master branch (TrinityCore/TrinityCore@65f82c7#diff-ad66842f3a1bb1684807e622c9f129d330fa7cd3daf4ba9a62254cd2de340c52); It's just for information, not necessary for this PR.

Sorry, I don't quite understand. Are you saying you want me to merge in that TC commit?

Not necessary. If you want other improvements for grids, it's another possibility. But no real impact IG. Just code management.

Takenbacon · 2025-01-21T12:22:49Z

I see that you have moved some grid definitions outside of maps. If you don't these data are duplicated at different places in the framework, and it can be merged. I've a PR that port these changes available only on TC master branch (TrinityCore/TrinityCore@65f82c7#diff-ad66842f3a1bb1684807e622c9f129d330fa7cd3daf4ba9a62254cd2de340c52); It's just for information, not necessary for this PR.

Sorry, I don't quite understand. Are you saying you want me to merge in that TC commit?

Not necessary. If you want other improvements for grids, it's another possibility. But no real impact IG. Just code management.

Ah gotcha. Yeah, I still have a bunch of local WIP changes to reorganize some more of the code. But I am definitely starting to get tired of messing with it so I might just end up finishing what I'm doing and calling it "good" for now, haha.

Nyeriah · 2025-01-21T15:24:22Z

Every small improvement is a good improvement, absolutelly no need to address all that in one go 😅 Thanks for this all other PRs btw.

Takenbacon · 2025-01-21T19:10:39Z

My most recent commit will be the bulk of the refactoring I'll do in regards to grids I think. There's still a couple smaller things I'm planning on looking into cleaning up/improving but I could probably spend the rest of my life on this and I'd like to move on to other things eventually haha. It does compile currently (on MSVC) and I was able to login and test a couple things. I will be out of town this upcoming weekend so I'll hopefully get this wrapped up the following if all goes well.

Takenbacon added 2 commits December 17, 2024 13:05

WIP grid improvements

c2331f1

silly mistakes

203325d

github-actions bot added CORE Related to the core file-cpp Used to trigger the matrix build labels Dec 17, 2024

heyitsbench added the WIP Work in Progress. label Dec 17, 2024

Takenbacon added 3 commits December 18, 2024 04:36

Fix include

b471dc0

Merge branch 'master' into grid_improvements

7abdeec

Remove redundant AddObjectHelpers

2327425

Grimdhex reviewed Dec 20, 2024

View reviewed changes

Takenbacon added 5 commits December 22, 2024 16:51

Cleanup unused code in Grid.h

8addb49

Fix crash at grid unload

6dd56e4

Should be a continue and not a return, oops

85c7ae2

Move GridMap structure to a new grid terrain class. Needs more struct…

0f4ae6d

…ural changes

Merge branch 'master' into grid_improvements

69cba24

More refactoring

daae43b

github-actions bot added the Script label Jan 21, 2025

Takenbacon added 3 commits January 21, 2025 15:58

Misc changes/fixes

e761e30

Couple missed changes

22eefc3

Log total map cells too

3788f0f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(Core/Grids): Grid improvements #20955

fix(Core/Grids): Grid improvements #20955

Takenbacon commented Dec 17, 2024

Grimdhex commented Dec 18, 2024

Takenbacon commented Dec 18, 2024

Kitzunu commented Dec 18, 2024

Grimdhex Dec 20, 2024

Takenbacon Dec 21, 2024

Grimdhex commented Jan 21, 2025 •

edited

Loading

Takenbacon commented Jan 21, 2025

Grimdhex commented Jan 21, 2025

Takenbacon commented Jan 21, 2025

Nyeriah commented Jan 21, 2025

Takenbacon commented Jan 21, 2025

fix(Core/Grids): Grid improvements #20955

Are you sure you want to change the base?

fix(Core/Grids): Grid improvements #20955

Conversation

Takenbacon commented Dec 17, 2024

Changes Proposed:

Issues Addressed:

SOURCE:

Tests Performed:

How to Test the Changes:

Known Issues and TODO List:

How to Test AzerothCore PRs

Grimdhex commented Dec 18, 2024

Takenbacon commented Dec 18, 2024

Kitzunu commented Dec 18, 2024

Grimdhex Dec 20, 2024

Choose a reason for hiding this comment

Takenbacon Dec 21, 2024

Choose a reason for hiding this comment

Grimdhex commented Jan 21, 2025 • edited Loading

Takenbacon commented Jan 21, 2025

Grimdhex commented Jan 21, 2025

Takenbacon commented Jan 21, 2025

Nyeriah commented Jan 21, 2025

Takenbacon commented Jan 21, 2025

Grimdhex commented Jan 21, 2025 •

edited

Loading