feat: selective packet disabling#3804
Conversation
There are some monk behaviors that are not reflecting reality. I also took the opportunity to align the wording with tibia official terms for monk harmony manipulation: spend and build. - The whole party is being healed - Spender spells are not healing - Spenders are not building, when using harmony virtue - Harmony healing is constant, regardless of how many charges are being built - Spenders also heals - Spenders should also build, when using harmony virtue - Party healing impacts only the lower-health player, not the whole party - Harmony passive healing is proportional to the amount of harmony built/spent Co-Authored-By: murilo09 <78226931+murilo09@users.noreply.github.com> Co-Authored-By: Pedro Cruz <phac@cin.ufpe.br> Co-Authored-By: Felipe <87909998+FelipePaluco@users.noreply.github.com> Co-Authored-By: Vava <110659678+vavasz@users.noreply.github.com>
Replaced static Spectators::find call with instance method and updated callback execution to remove redundant argument. This improves code clarity and aligns with updated Spectators and callback APIs.
📝 WalkthroughWalkthroughProtocol messaging was reorganized: several send helpers were renamed (e.g., ChangesMonk State Messaging
Passive Cooldown System
Bestiary & Imbuement APIs
Protocol Feature Guards & Helper Renames
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/server/network/protocol/protocolgame.cpp (2)
3450-3466:⚠️ Potential issue | 🟠 MajorEnsure PROTOCOL_DISABLE_UPDATE_ICONS suppresses icon payload in AddCreature
AddCreatureIconis still invoked during creature add, so icon bytes are still emitted even whenPROTOCOL_DISABLE_UPDATE_ICONSis defined. This defeats selective disabling (and could still trigger the crash you’re isolating). Consider sending a zero count when the feature is disabled so the packet layout stays valid.🧩 Suggested fix (keep field, but zero it when disabled)
void ProtocolGame::AddCreatureIcon(NetworkMessage &msg, const std::shared_ptr<Creature> &creature) { - - if (!creature || !player || oldProtocol) { + if (oldProtocol) { return; } +#ifdef PROTOCOL_DISABLE_UPDATE_ICONS + msg.addByte(0); + return; +#endif + if (!creature || !player) { + return; + } const auto icons = creature->getIcons(); // client only supports 3 icons, otherwise it will crash const auto count = icons.size() > 3 ? 3 : icons.size(); msg.addByte(count);Also applies to: 8425-8426
8722-8750:⚠️ Potential issue | 🟠 MajorGuard null imbuement lookups to avoid crashes
getImbuement/getBaseByID/getCategoryByIDare dereferenced without null checks. If data is invalid or mismatched (e.g., DB corruption), this becomes a hard crash. Consider early returns with logging.🛡️ Suggested null-guard
void ProtocolGame::AddImbuementInfo(NetworkMessage &msg, uint16_t imbuementId) const { Imbuement* imbuement = g_imbuements().getImbuement(imbuementId); + if (!imbuement) { + g_logger().warn("[ProtocolGame::AddImbuementInfo] Unknown imbuement id {}", imbuementId); + return; + } const BaseImbuement* baseImbuement = g_imbuements().getBaseByID(imbuement->getBaseID()); const CategoryImbuement* categoryImbuement = g_imbuements().getCategoryByID(imbuement->getCategory()); + if (!baseImbuement || !categoryImbuement) { + g_logger().warn("[ProtocolGame::AddImbuementInfo] Missing base/category for imbuement id {}", imbuementId); + return; + }
🧹 Nitpick comments (5)
src/creatures/players/components/wheel/player_wheel.cpp (1)
1548-1552: Replace magic passive id with a named constantKeeps the passive-id mapping centralized and avoids accidental drift.
Proposed tweak
void PlayerWheel::sendGiftOfLifeCooldown() const { - // gift of life passive id is 1 - bool paused = m_player.getZoneType() == ZONE_PROTECTION || !m_player.hasCondition(CONDITION_INFIGHT); - m_player.sendPassiveCooldown(1, getGiftOfCooldown(), getGiftOfLifeTotalCooldown(), paused); + constexpr uint8_t kGiftOfLifePassiveId = 1; + const bool paused = m_player.getZoneType() == ZONE_PROTECTION || !m_player.hasCondition(CONDITION_INFIGHT); + m_player.sendPassiveCooldown(kGiftOfLifePassiveId, getGiftOfCooldown(), getGiftOfLifeTotalCooldown(), paused); }src/creatures/players/player.hpp (1)
908-908: Clarify cooldown units in the new APIA short doc comment noting whether values are milliseconds/seconds (and what
pausedmeans) would prevent misuse.src/utils/definitions.hpp (1)
12-142: Consider moving the disable toggles to a dedicated sample header or docsKeeping a big commented block in a core header can become stale and create merge churn. A separate sample file (included only when needed) or documentation would keep production headers cleaner.
src/server/network/protocol/protocolgame.hpp (1)
422-425: NewsendPassiveCooldowndeclaration fits naturally alongside existing cooldown methods.The signature and placement next to
sendSpellCooldown/sendSpellGroupCooldown/sendUseItemCooldownare consistent. One minor observation: consider whether this method could beconst-qualified likeAddImbuementInfoandAddForgeSkillStats, since sending a cooldown packet typically shouldn't mutateProtocolGamestate. If the implementation only writes to the output buffer (which takes a non-const ref), this may not be feasible — just worth a quick check.src/server/network/protocol/protocolgame.cpp (1)
10373-10436: Cap OTCR outfit feature counts to 0xFFThese counts are serialized as
uint8_t, but the loop bounds areuint16_tmax. If feature lists ever exceed 255, the count will truncate while data keeps growing, desyncing the packet. Consider clamping touint8_tfor wings/auras/effects.♻️ Example adjustment (apply similarly to auras/effects)
- uint16_t limitWings = std::numeric_limits<uint16_t>::max(); - uint16_t wingSize = 0; + uint8_t limitWings = std::numeric_limits<uint8_t>::max(); + uint8_t wingSize = 0; msg.skipBytes(1); const auto &wings = g_game().getAttachedEffects()->getWings(); for (const auto &wing : wings) { if (player->attachedEffects().hasWing(wing)) { msg.add<uint16_t>(wing->id); msg.addString(wing->name); ++wingSize; } - if (wingSize == limitWings) { + if (wingSize == limitWings) { break; } }
|
|
This PR is stale because it has been open 45 days with no activity. |
|
@copilot resolve the merge conflicts in this pull request |
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/creatures/players/player.cpp (1)
8096-8100: ⚡ Quick winAdd a focused build check for both packet-disable macro states.
These compile-time branches are easy to regress silently. Add a minimal CI/build step that compiles once with default flags and once with
PROTOCOL_DISABLE_LUA_MESSAGES/PROTOCOL_DISABLE_HUNTING_TASKSenabled.As per coding guidelines,
For C++ changes, prefer focused tests or the smallest practical build/check that validates the touched code.Also applies to: 9780-9785
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/creatures/players/player.cpp` around lines 8096 - 8100, The code path guarded by PROTOCOL_DISABLE_LUA_MESSAGES (and similarly PROTOCOL_DISABLE_HUNTING_TASKS around other branches) can regress if not compiled under both macro states; add a minimal CI/build check that compiles the project twice (or adds a matrix job) once with default flags and once with -DPROTOCOL_DISABLE_LUA_MESSAGES and -DPROTOCOL_DISABLE_HUNTING_TASKS set, so the compilation covers the branch around player.cpp where client->writeToOutputBuffer(message) is conditionally compiled (also apply the same check for the other branch at the referenced area around lines ~9780-9785); ensure the new job only performs a fast compile (no tests) and fails the build on compilation errors.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/creatures/players/player.cpp`:
- Around line 8096-8100: The code path guarded by PROTOCOL_DISABLE_LUA_MESSAGES
(and similarly PROTOCOL_DISABLE_HUNTING_TASKS around other branches) can regress
if not compiled under both macro states; add a minimal CI/build check that
compiles the project twice (or adds a matrix job) once with default flags and
once with -DPROTOCOL_DISABLE_LUA_MESSAGES and -DPROTOCOL_DISABLE_HUNTING_TASKS
set, so the compilation covers the branch around player.cpp where
client->writeToOutputBuffer(message) is conditionally compiled (also apply the
same check for the other branch at the referenced area around lines ~9780-9785);
ensure the new job only performs a fast compile (no tests) and fails the build
on compilation errors.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 32831779-7b48-494c-8cb2-2be080c2ea20
📒 Files selected for processing (6)
src/creatures/combat/condition.cppsrc/creatures/players/components/wheel/player_wheel.cppsrc/creatures/players/player.cppsrc/creatures/players/player.hppsrc/server/network/protocol/protocolgame.cppsrc/server/network/protocol/protocolgame.hpp
🚧 Files skipped from review as they are similar to previous changes (5)
- src/creatures/combat/condition.cpp
- src/creatures/players/components/wheel/player_wheel.cpp
- src/creatures/players/player.hpp
- src/server/network/protocol/protocolgame.hpp
- src/server/network/protocol/protocolgame.cpp
|
This PR is stale because it has been open 45 days with no activity. |
2 participants
Description
Adds definitions to selectively disable packets.
This is very useful for when you have problems with the game client crashing on login to identify the cause.
IMPORTANT: This PR tagets the branch dudantas/feat-monk and will merge to it when accepted
I used that branch as a base for this pr because #3794 involves big protocolgame changes. If I used main branch, either this pr or 3794 would have to remade to fix merge conflicts.
Before merging, ensure that you have everything pushed to 3794.
After merging do a pull on your local repository in order to work on updated version of this branch.
Type of change
How Has This Been Tested
Checklist
Summary by CodeRabbit
New Features
Improvements