mod-multibot-bridge: Native bridge endpoints and inventory enhancements - #15
Open
Jellypowered wants to merge 3 commits into
Open
mod-multibot-bridge: Native bridge endpoints and inventory enhancements#15Jellypowered wants to merge 3 commits into
Jellypowered wants to merge 3 commits into
Conversation
…pells, talents, and targeted crafting New endpoints: QUEST_ABANDON, QUEST_SHARE, ITEM_EQUIP, ITEM_TRADE, CAST_SPELL, TALENT_APPLY, CRAFT_RECIPE_TARGET. New inventory packets: INV_BAG, INV_ITEM_LOC, INV_EQUIP_LOC. Fixes: quiver/ammo pouch equip, quest share group-wide, spell cast error reporting, talent apply native path. Fully additive; preserves MultiBot-Chatless compatibility.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAjout de nouvelles commandes RUN, de deux endpoints GET bulk, de paquets INV localisés, et du routage associé dans ChangesExtension du protocole bridge MultiBotBridge
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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.
Inline comments:
In `@src/MultiBotBridge.cpp`:
- Around line 5584-5601: The SendInventoryBulkPackets function currently only
sends inventory summary data via INV_BULK_ITEM packets but is missing the actual
inventory item location details. After building the InventorySummaryData and
sending the summary via INV_BULK_ITEM, you must also send INV_BAG and
INV_ITEM_LOC packets for each bot to expose the actual inventory locations and
items as part of the bulk inventory response. Add these additional packet sends
within the loop for each bot returned by GetBridgeVisibleBots to complete the
contract with the expected bulk inventory functionality.
- Around line 3336-3338: The selection restoration after the check/cast
operation is conditional on oldSelection being non-empty, which leaves a
persistent forced target when the bot had no prior selection. Remove the if
condition checking oldSelection in the SetSelection call within the relevant
code blocks, and instead call bot->SetSelection(oldSelection) unconditionally to
ensure the selection is always restored to its original state, whether it was
empty or not. This same fix must be applied at both occurrences mentioned in the
comment.
- Around line 3948-3964: The inventory action loop in the code starting with
FindBagItemByEntry does not respect the requested count strictly because it
moves entire item stacks and only checks if moved >= requestedCount after the
fact, potentially exceeding the requested amount. Before moving items in this
loop and in the similar deposit/withdrawal/purchase paths mentioned in the
comment, calculate how many items should actually be moved to not exceed
requestedCount (for example, if requestedCount is positive and moving the entire
stackCount would exceed it, only move the difference needed). Apply this
validation logic before calling RemoveItem and BankItem (or equivalent move
operations in the other paths) to ensure the total moved quantity never exceeds
the originally requested count.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 33414ba7-789a-49be-9889-9ac91bde6168
📒 Files selected for processing (2)
README.mdsrc/MultiBotBridge.cpp
Author
|
Woah, that's fancy! |
Owner
|
Thank you for the substantial contribution. The native chatless endpoints are aligned with the project direction, but the current PR combines several independent and high-risk write paths in one large change. Before merging, we need it rebased on the current bridge main branch and split into smaller PRs. Inventory stack splitting, native quest state changes, direct spell construction, talent reset/apply semantics, trade transactionality, bulk packet limits, and request validation all require targeted review and tests. Please do not close the work; we would like to evaluate and integrate the safe parts incrementally. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds new native bridge endpoints for quest management, item actions, spell casting, talent applying, and targeted profession crafting. Improves inventory data with bag entries and exact item locations. Fixes quiver/ammo pouch equip support and refactors legacy bank/guild-bank helper functions.
Important notes
New endpoints
RUN~QUEST_ABANDONRUN~QUEST_SHARERUN~ITEM_EQUIPAUTO,BAG,MAIN_HAND,OFF_HAND,RANGED)RUN~ITEM_TRADERUN~CAST_SPELLSpellCastResultmapping)RUN~TALENT_APPLY0-0-0)RUN~CRAFT_RECIPE_TARGETNew inventory data packets
INV_BAGINV_ITEM_LOCbag~slotlocation for each inventory itemINV_EQUIP_LOCHelper functions
SendRunResult()– unified run-result payloadParseUint32Field(),ParseUint8Field()– safe integer parsingSameName()– case-insensitive name comparisonFindQuestLogSlot()– quest log slot lookupFindAllowedPlayerTarget()– resolve target for spells/tradeResolveSpellTarget()– smart spell target resolution (selected, named, AI possible/all targets)CheckBridgeSpellCast()– full validation before cast (returns actualSpellCastResult)CastBridgeSpellDirect()– direct spell casting with validated targetsFindItemByOptionalPosition()– item lookup by optional bag/slotBuildBagEntries()– build bag list for inventory viewGetBagTypeString()– classify bags (QUIVER, AMMO_POUCH, etc.)ResolveEquipSlot()– resolve destination slot with container/bag supportEquipItemToSlot()– equip item to resolved slotFixes/improvements
ITEM_EQUIPnow acceptsITEM_CLASS_CONTAINER, routes to bag slotsSpellCastResult→ bridge reason mapping (no more genericTRY_AGAIN)talents apply <link>Breaking changes
None. All changes are additive; existing endpoints remain unchanged.
Testing notes
luac -p) (kind of a moot since this module isn't where the LUA was but ok 😆 )git diff --check)Files changed
src/MultiBotBridge.cpp: ~1,700 lines added, legacy helper functions preservedAI assistance/Disclaimer
AI assistance was utilized to compose this PR message and to help identify bridge hooks/functions for extending the protocol. The implementation follows existing patterns and conventions in
MultiBotBridge.cpp. This was not "vibe coded", but it goes without saying people get a little testy about AI usage. I did test these endpoints extensively while developing, this is several iterations worth of work over a couple of weeks to get it to what I felt was a PR ready state.Thanks for taking over MultiBot and creating this wonderful module!
Jelly
Summary by CodeRabbit