Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors the RPG status tracking system from using a C-style union to a modern C++ std::variant. The refactoring improves type safety by replacing direct union member access with variant-based access patterns using std::get and std::get_if.
Changes:
- Replaced union-based storage with
std::variant<Idle, GoGrind, GoCamp, WanderNpc, WanderRandom, DoQuest, Rest, TravelFlight>for RPG state data - Added
GetStatus()method to map variant types to enum values - Updated all RPG info access patterns to use
std::get(in switch cases) andstd::get_if(for safe access with null checks) - Reorganized
NewRpgStatusenum to placeRPG_IDLEat index 0 (was previously at index 7) and removed redundantRPG_STATUS_START - Removed
RPG_INFOmacro in favor of direct variant access
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/PlayerbotAIConfig.h | Reorganized NewRpgStatus enum with RPG_IDLE as the initial status (0), removed RPG_STATUS_START |
| src/Ai/World/Rpg/NewRpgInfo.h | Replaced union with std::variant, added GetStatus() method, removed RPG_INFO macro |
| src/Ai/World/Rpg/NewRpgInfo.cpp | Implemented GetStatus() using std::visit, refactored ToString() to use variant patterns, simplified Change* methods |
| src/Ai/World/Rpg/Action/NewRpgAction.h | Updated method signatures to pass variant data by reference |
| src/Ai/World/Rpg/Action/NewRpgAction.cpp | Updated all variant access to use std::get in switch statements and std::get_if for safe nullable access |
| src/Ai/World/Rpg/Trigger/NewRpgTrigger.cpp | Updated to use GetStatus() method instead of direct status field access |
| src/Bot/RandomPlayerbotMgr.cpp | Updated to use GetStatus() method for statistics tracking |
| src/Ai/Base/Value/GrindTargetValue.cpp | Updated to use GetStatus() method for status checking |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| using RpgData = std::variant< | ||
| Idle, | ||
| GoGrind, | ||
| GoCamp, | ||
| WanderNpc, | ||
| WanderRandom, | ||
| DoQuest, | ||
| Rest, | ||
| TravelFlight | ||
| >; | ||
| RpgData data; |
There was a problem hiding this comment.
The code uses std::variant at line 75 but the header does not appear to include . While this may work if is included indirectly through one of the other headers, it's a best practice to explicitly include all headers for types used directly in a file. Consider adding '#include ' to make the dependency explicit and avoid potential compilation issues in the future.
| struct NewRpgInfo | ||
| { | ||
| NewRpgInfo() {} | ||
| NewRpgInfo() : data(Idle{}) {} |
There was a problem hiding this comment.
The PR description template was not filled in. The PR should include:
- A description of what this change does and why it's needed
- How to test the changes
- Answers to the complexity and impact questions
- Whether AI assistance was used
- Whether the final checklist items have been verified
While the code changes appear to be a refactoring from union to std::variant, this information should be documented in the PR description for reviewers and future reference.
| WanderNpc, | ||
| WanderRandom, | ||
| DoQuest, | ||
| Rest, | ||
| TravelFlight |
There was a problem hiding this comment.
The order of types in the RpgData variant doesn't match the order of the corresponding enum values in NewRpgStatus. While this doesn't cause functional issues due to the GetStatus() method using std::is_same_v for type checking, it creates unnecessary maintenance burden and could confuse future developers. Consider reordering the variant types to match the enum order (Idle, GoGrind, GoCamp, WanderRandom, WanderNpc, DoQuest, TravelFlight, Rest) for better code clarity and consistency.
| WanderNpc, | |
| WanderRandom, | |
| DoQuest, | |
| Rest, | |
| TravelFlight | |
| WanderRandom, | |
| WanderNpc, | |
| DoQuest, | |
| TravelFlight, | |
| Rest |
| flight.fromFlightMaster = fromFlightMaster; | ||
| flight.fromNode = fromNode; | ||
| flight.toNode = toNode; | ||
| flight.inFlight = false; |
There was a problem hiding this comment.
The explicit initialization of inFlight to false is redundant since the TravelFlight struct already initializes inFlight to false by default (see line 55 in NewRpgInfo.h). This line can be removed for cleaner code.
| flight.inFlight = false; |
| if (!dataPtr) | ||
| return false; | ||
|
|
||
| auto& data = *dataPtr; |
There was a problem hiding this comment.
Incorrect indentation: this line should be aligned with the previous lines (not indented with extra spaces). It should be at the same indentation level as line 432.
| auto& data = *dataPtr; | |
| auto& data = *dataPtr; |
3 participants
Pull Request
Describe what this change does and why it is needed...
Design Philosophy
We prioritize stability, performance, and predictability over behavioral realism.
Complex player-mimicking logic is intentionally limited due to its negative impact on scalability, maintainability, and
long-term robustness.
Excessive processing overhead can lead to server hiccups, increased CPU usage, and degraded performance for all
participants. Because every action and
decision tree is executed per bot and per trigger, even small increases in logic complexity can scale poorly and
negatively affect both players and
world (random) bots. Bots are not expected to behave perfectly, and perfect simulation of human decision-making is not a
project goal. Increased behavioral
realism often introduces disproportionate cost, reduced predictability, and significantly higher maintenance overhead.
Every additional branch of logic increases long-term responsibility. All decision paths must be tested, validated, and
maintained continuously as the system evolves.
If advanced or AI-intensive behavior is introduced, the default configuration must remain the lightweight decision
model. More complex behavior should only be
available as an explicit opt-in option, clearly documented as having a measurable performance cost.
Principles:
Stability before intelligence
A stable system is always preferred over a smarter one.
Performance is a shared resource
Any increase in bot cost affects all players and all bots.
Simple logic scales better than smart logic
Predictable behavior under load is more valuable than perfect decisions.
Complexity must justify itself
If a feature cannot clearly explain its cost, it should not exist.
Defaults must be cheap
Expensive behavior must always be optional and clearly communicated.
Bots should look reasonable, not perfect
The goal is believable behavior, not human simulation.
Before submitting, confirm that this change aligns with those principles.
Feature Evaluation
Please answer the following:
How to Test the Changes
Complexity & Impact
Does this change add new decision branches?
Does this change increase per-bot or per-tick processing?
Could this logic scale poorly under load?
Defaults & Configuration
If this introduces more advanced or AI-heavy logic:
AI Assistance
If yes, please specify:
AI assistance is allowed, but all submitted code must be fully understood, reviewed, and owned by the contributor.
Any AI-influenced changes must be verified against existing CORE and PB logic. We expect contributors to be honest
about what they do and do not understand.
Final Checklist
Notes for Reviewers
Anything that significantly improves realism at the cost of stability or performance should be carefully discussed
before merging.