Skip to content

Core/Garrisons: Allow players to have multiple garrisons (one per type)#31636

Open
Golrag wants to merge 4 commits intoTrinityCore:masterfrom
Golrag:garrisons_multiple
Open

Core/Garrisons: Allow players to have multiple garrisons (one per type)#31636
Golrag wants to merge 4 commits intoTrinityCore:masterfrom
Golrag:garrisons_multiple

Conversation

@Golrag
Copy link
Contributor

@Golrag Golrag commented Jan 24, 2026

Changes proposed:

  • Allow storing multiple garrisons for players (one per type)

Issues addressed:

Closes:

Tests performed:

  • Builds locally
  • Alliance player -> unlock shadowmoon garrison (quest completion)
  • Entering Garrison
  • Exiting Garrison still crashes (was the case before these changes)

Known issues and TODO list:

  • I don't really like the code, not sure if we should keep the followers and buildings per garrison type or not, might bloat the PR and make review harder
  • Not sure how to handle deletion

@Golrag Golrag force-pushed the garrisons_multiple branch from f7e1649 to c623b98 Compare January 29, 2026 18:09
#include "Player.h"
#include "VehicleDefines.h"
#include "advstd.h"
#include <ranges>
Copy link
Member

@Shauren Shauren Feb 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

<ranges> is banned

@Shauren
Copy link
Member

Shauren commented Feb 2, 2026

Why did you decide to make Garrison class a giant bag containing all garrisons at the same time instead of multiple Garrison objects in Player?

@Golrag
Copy link
Contributor Author

Golrag commented Feb 2, 2026

I wanted to have an class that would handle and/or delegate to the correct garrison. So that player->GetGarrison()->AddFollower would work without needing to know to which garrison said follower exists (at least not in the place calling this). Whether or not this was a good idea given the usecases in CriteriaHandler is something im not sure about

I had thoughts about either introducing a new class in between them, or moving the followers, buildings etc to the GarrisonInfo class, but decided not to (yet) as I wanted more feedback on whatever mess I already had and get some more ideas and opinions

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants