feat: blood brothers quest#3978
Draft
Paco161315 wants to merge 8 commits into
Draft
Conversation
WARNING: Don't merge this pull request Until Majesty upload the map Update I sent to him or Quest will be broken. Complete Blood Brothers Quest + Questlog Every detail has been created, achievements, dialogs and more. This PR has been debuged and tested at https://tibiatales.com
📝 WalkthroughWalkthroughAdds the Blood Brothers quest: updated storages and catalog; 8 new monster scripts; many NPC dialogue and keyword integrations; quest actions, movement events, and death handlers; plus system integrations (skinning, baking, login/achievements, item attributes). ChangesBlood Brothers Quest Feature
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 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)
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements the 'Blood Brothers' quest, introducing new monsters, NPCs, and mechanics like map marking and boss encounters, while also updating spell teaching and skinning systems. The review identifies critical logic errors in storage value handling where 'or 0' fallbacks fail because unset values return -1, which is truthy in Lua, causing off-by-one errors in quest progress. Additionally, a potential memory leak was found in the vampire bride death script due to global-scope tables that are not consistently reset.
Comment on lines
+1
to
+2
| local vampireBrideKills = {} | ||
| local killCooldown = {} |
There was a problem hiding this comment.
The use of global-scope local tables vampireBrideKills and killCooldown to track quest progress in a specific area is problematic. These tables are only cleared when the teleport condition is met (killCount >= 2). If a player kills only one bride and leaves, the entry remains in the table indefinitely, leading to a memory leak and potentially triggering the teleport for the next player who kills just one bride. Consider using a room-based state manager or checking for active players in the area to reset these values.
| if marks == 6 then | ||
| text = "seven" | ||
| elseif marks == 7 then -- entrace does not count | ||
| text = "eight" |
There was a problem hiding this comment.
The logic player:getStorageValue(BB.MapMarks) or 0 will not work as expected because getStorageValue returns -1 when a storage key is not set. In Lua, -1 is truthy, so the expression will evaluate to -1 instead of 0. This will cause issues in the subsequent checks for the number of marks.
local marks = math.max(0, player:getStorageValue(BB.MapMarks))
| player:setStorageValue(loc.markStorage, 1) | ||
| player:setStorageValue(STORAGE_MAPMARKS, (player:getStorageValue(STORAGE_MAPMARKS) or 0) + 1) | ||
| player:say("You marked the " .. loc.name .. ".", TALKTYPE_MONSTER_SAY) | ||
| return true |
There was a problem hiding this comment.
Similar to the issue in julius.lua, (player:getStorageValue(STORAGE_MAPMARKS) or 0) will evaluate to -1 if the storage is unset. Adding 1 to -1 results in 0, which means the first mark recorded will be 0 instead of 1. This off-by-one error propagates to the NPC dialogue logic.
player:setStorageValue(STORAGE_MAPMARKS, math.max(0, player:getStorageValue(STORAGE_MAPMARKS)) + 1)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
♻️ Duplicate comments (1)
data-otservbr-global/scripts/quests/blood_brothers_quest/actions_lersatio_mirrors.lua (1)
66-66:⚠️ Potential issue | 🟠 MajorVerify spectator range calculation.
The
Game.getSpectatorscall has the same issue as inactions_ghost_tear.luaLine 24. It usestoPos.x - fromPos.xfor both min and max range parameters, which retrieves spectators within a radius fromfromPosrather than the rectangular area fromfromPostotoPos.🤖 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 `@data-otservbr-global/scripts/quests/blood_brothers_quest/actions_lersatio_mirrors.lua` at line 66, The Game.getSpectators call uses the same value for min and max x/y ranges so it behaves like a radius from fromPos; compute explicit left/right/up/down distances from fromPos to toPos instead and pass those to Game.getSpectators. Concretely, calculate right = math.max(0, toPos.x - fromPos.x), left = math.max(0, fromPos.x - toPos.x), down = math.max(0, toPos.y - fromPos.y), up = math.max(0, fromPos.y - toPos.y) and call Game.getSpectators(fromPos, false, false, right, left, down, up) (using the same fromPos/toPos and spectators variable) so the function covers the rectangular area between fromPos and toPos.
🧹 Nitpick comments (8)
data-otservbr-global/scripts/creaturescripts/monster/vampire_bride_death.lua (3)
19-24: 💤 Low valueRedundant player check.
The
spectator:isPlayer()check on line 20 is redundant becauseGame.getSpectators()is already called withonlyPlayer = trueon line 17, meaningspectatorswill only contain players.♻️ Simplify by removing redundant check
for _, spectator in ipairs(spectators) do - if spectator:isPlayer() then - spectator:teleportTo(teleportDest) - teleportDest:sendMagicEffect(CONST_ME_TELEPORT) - end + spectator:teleportTo(teleportDest) + teleportDest:sendMagicEffect(CONST_ME_TELEPORT) end🤖 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 `@data-otservbr-global/scripts/creaturescripts/monster/vampire_bride_death.lua` around lines 19 - 24, Remove the redundant spectator:isPlayer() conditional inside the loop since Game.getSpectators was called with onlyPlayer = true; update the loop over spectators (the variable spectators returned by Game.getSpectators) to directly call spectator:teleportTo(teleportDest) and teleportDest:sendMagicEffect(CONST_ME_TELEPORT) for each spectator, leaving Game.getSpectators(...) and teleportDest references intact.
1-2: Module-level state tables may reset on script reload.The
vampireBrideKillsandkillCooldowntables are module-level and will be cleared if the script is reloaded (e.g., via/reload scripts). This could disrupt active encounters.For production robustness, consider:
- Using global storage via
GlobalStoragefor persistence across reloads- Adding server-restart cleanup logic
- Documenting that active encounters require a server restart to reset
🤖 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 `@data-otservbr-global/scripts/creaturescripts/monster/vampire_bride_death.lua` around lines 1 - 2, The module-level tables vampireBrideKills and killCooldown can be lost on script reload; replace them with persistent storage via GlobalStorage (or equivalent server-wide storage) by introducing stable keys (e.g., "vampireBrideKills" and "vampireBrideKillCooldowns") and helper functions (get/set/clear) to read/write these maps instead of using the in-file tables, update any references in functions that read or modify vampireBrideKills/killCooldown to call those helpers, and add an initialization/cleanup path that migrates existing in-memory state into GlobalStorage on load and clears or resets entries on server restart as needed; document behavior in comments so maintainers know reloads won't preserve in-memory encounter state.
26-27: ⚡ Quick winAdd error handling for monster spawning.
Game.createMonster()can returnnilif spawning fails (e.g., invalid position, blocked tile). Without validation, the quest mechanic could fail silently.🛡️ Add spawn validation
- Game.createMonster("Marziel", spawnPos) - Game.createMonster("Vampire", Position(spawnPos.x - 1, spawnPos.y, spawnPos.z)) + local marziel = Game.createMonster("Marziel", spawnPos) + local vampire = Game.createMonster("Vampire", Position(spawnPos.x - 1, spawnPos.y, spawnPos.z)) + + if not marziel or not vampire then + logger.error("[VampireBrideDeath] Failed to spawn boss monsters at position: {}", spawnPos) + return true + end🤖 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 `@data-otservbr-global/scripts/creaturescripts/monster/vampire_bride_death.lua` around lines 26 - 27, Game.createMonster calls can return nil on failure, so after calling Game.createMonster("Marziel", spawnPos) and Game.createMonster("Vampire", Position(...)) check each return value and handle nil: if spawn failed, log an error (including monster name and spawnPos), abort or rollback the quest flow (so later logic doesn’t assume the monsters exist), and optionally try alternative nearby positions or notify the player; reference the Game.createMonster calls and the spawnPos / Position(...) used so you update the spawning block to validate results and handle failures safely.data-otservbr-global/scripts/creaturescripts/monster/boreth_death.lua (1)
7-18: ⚡ Quick winConsider using
Game.getSpectators()for better performance.The nested loop creates a
Tileobject and callsgetCreatures()for each position. UsingGame.getSpectators()to scan the entire area at once would be more efficient.⚡ Proposed refactor using Game.getSpectators()
- for x = fromPos.x, toPos.x do - for y = fromPos.y, toPos.y do - local tile = Tile(Position(x, y, 1)) - if tile then - for _, spectator in ipairs(tile:getCreatures()) do - if spectator:isMonster() and spectator:getName():lower() == "plaguethrower" then - spectator:remove() - end - end - end - end - end + local centerPos = Position((fromPos.x + toPos.x) / 2, (fromPos.y + toPos.y) / 2, 1) + local rangeX = math.floor((toPos.x - fromPos.x) / 2) + local rangeY = math.floor((toPos.y - fromPos.y) / 2) + local spectators = Game.getSpectators(centerPos, false, false, rangeX, rangeX, rangeY, rangeY) + + for _, spectator in ipairs(spectators) do + if spectator:isMonster() and spectator:getName():lower() == "plaguethrower" then + spectator:remove() + end + end🤖 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 `@data-otservbr-global/scripts/creaturescripts/monster/boreth_death.lua` around lines 7 - 18, Replace the nested for-loops and repeated Tile(Position)/tile:getCreatures() calls with a single Game.getSpectators() call that covers the rectangle fromPos..toPos; compute the center Position and x/y ranges from fromPos and toPos, call Game.getSpectators(center, false, false, rangeX, rangeY) and then iterate those spectators, keeping the existing checks spectator:isMonster() and spectator:getName():lower() == "plaguethrower" and calling spectator:remove() for matches; this removes creating many Tile objects and improves performance while preserving the same logic around fromPos, toPos, Position, spectator:isMonster(), spectator:getName(), and spectator:remove().data-otservbr-global/npc/zarak.lua (1)
48-180: ⚡ Quick winRefactor repetitive spell node creation into a declarative registration pattern.
A spell-definition table plus loop would keep this easier to validate and reduce accidental metadata mismatches during future edits.
🤖 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 `@data-otservbr-global/npc/zarak.lua` around lines 48 - 180, The file has many repetitive keywordHandler:addKeyword + nodeX:addChildKeyword calls (node1..node27) registering spells via StdModule.learnSpell; refactor by creating a declarative spells table (each entry: trigger, text, spellName, vocation, price, level, premium, onlyFocus/premium flags) and iterate it to call keywordHandler:addKeyword and addChildKeyword for each entry (preserve npcHandler, StdModule.say prompts and StdModule.learnSpell parameters exactly). Do the same optionally to derive the nodeLevels listings from the same table (keep existing nodeLevels keywords/phrasing from nodeLevels variable) so metadata (spellName, price, level, vocation, premium) stays single-source-of-truth and eliminates the node1..node27 duplication.data-otservbr-global/npc/tamara.lua (1)
53-325: ⚡ Quick winUse a table-driven spell registration helper to reduce drift risk.
The current node-by-node pattern is highly repetitive; moving spell definitions into a data table plus a small registrar helper would make edits safer and easier to review.
🤖 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 `@data-otservbr-global/npc/tamara.lua` around lines 53 - 325, The file repeats many nearly identical blocks (node1..node67 using keywordHandler:addKeyword with StdModule.say and StdModule.learnSpell) which is error-prone; refactor by creating a data table (array of spell entries with keys: trigger word, prompt text, spellName, vocation, price, level, premium flag) and a small registrar function (e.g., registerSpell(entry)) that calls keywordHandler:addKeyword and adds the "yes" child with StdModule.learnSpell; also convert the nodeLevels registrations (nodeLevels:addChildKeyword) into a similar data-driven list so the level messages are generated from a levels table—use existing symbols keywordHandler:addKeyword, StdModule.learnSpell, StdModule.say, nodeLevels to locate where to replace the repeated blocks.data-otservbr-global/npc/tamoril.lua (1)
113-374: ⚡ Quick winConsider consolidating spell declarations into structured data + loop.
This block is very copy/paste-heavy; a declarative table with one registration function would reduce maintenance errors and simplify future balance updates.
🤖 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 `@data-otservbr-global/npc/tamoril.lua` around lines 113 - 374, The file contains many repetitive keywordHandler:addKeyword + nodeX:addChildKeyword blocks (see node1..node63 and nodeLevels) registering spells with StdModule.say and StdModule.learnSpell; replace these repetitive blocks with a declarative table/array of spell entries (fields: keywords, text, premium, spellName, vocation, price, level) and a small loop that calls keywordHandler:addKeyword and then addChildKeyword for each entry (use the same StdModule.say and StdModule.learnSpell handlers), and likewise convert the nodeLevels child registrations into a data table iterated to register level responses; keep existing symbols keywordHandler:addKeyword, StdModule.say, StdModule.learnSpell and nodeLevels to locate where to swap in the loop.data-otservbr-global/scripts/quests/blood_brothers_quest/actions_lersatio_mirrors.lua (1)
68-73: 💤 Low valueRemove unused variable.
The
playersTeleportedcounter is incremented but never used. Either use it for logging/validation or remove it.♻️ Proposed fix to remove unused variable
- local playersTeleported = 0 for _, spectator in ipairs(spectators) do if spectator:isPlayer() then spectator:teleportTo(teleportDestination) spectator:getPosition():sendMagicEffect(CONST_ME_TELEPORT) - playersTeleported = playersTeleported + 1 end end🤖 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 `@data-otservbr-global/scripts/quests/blood_brothers_quest/actions_lersatio_mirrors.lua` around lines 68 - 73, The local variable playersTeleported is declared and incremented in the loop over spectators but never read; remove the declaration and the playersTeleported = playersTeleported + 1 line to avoid the unused variable, leaving the loop to call spectator:teleportTo(teleportDestination) and spectator:getPosition():sendMagicEffect(CONST_ME_TELEPORT) for each spectator; if you prefer to keep a count for validation or logging, instead use that counter (playersTeleported) after the loop to log or assert the expected number.
🤖 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 `@data-otservbr-global/lib/core/quests/catalog/init.lua`:
- Line 53: The catalog entry currently lists the module identifier
"052_blood_brothers" which doesn't match the actual module file name; update the
catalog array entry to "052_blood_brothers_quest" so the loader can find the
file (look for the string "052_blood_brothers" in the catalog init.lua and
replace it with "052_blood_brothers_quest").
In
`@data-otservbr-global/monster/quests/blood_brothers_quest/shadow_of_boreth.lua`:
- Around line 16-19: The shadow variants are reusing bosstiary.bossRaceId values
from their primary bosses (e.g., shadow_of_boreth.lua uses bossRaceId = 479 same
as boreth.lua), which will collide in server bosstiary lookup; either remove
monster.bosstiary from the summon-only shadow monsters or assign each shadow_*
monster a unique bosstiary.bossRaceId distinct from the primary (same approach
applies to shadow_of_marziel/shadow_of_lersatio and their primaries), update the
monster.bosstiary table accordingly so that bossRaceId values are unique per
monster or omitted for summons.
In `@data-otservbr-global/npc/armenius.lua`:
- Line 71: The conditional mixes OR/AND causing the branch to run for any
message when Mission03 == 2; update the check in npc/armenius.lua so the message
test and mission values are grouped together: ensure the expression uses
parentheses around (message:lower() == "alori mort" and
(player:getStorageValue(Storage.Quest.U8_4.BloodBrothers.Mission03) == 1 or
player:getStorageValue(Storage.Quest.U8_4.BloodBrothers.Mission03) == 2)) so
only the "alori mort" message triggers the branch when Mission03 is 1 or 2.
- Around line 87-105: After handling npcHandler:getTopic(playerId) == 5 and
performing the random branch actions (the MESSAGE_WALKAWAY branches,
teleport/spawn and achievement code), clear the NPC conversation topic for this
player so the same topic won't retrigger; call the NPC API to reset the topic
for playerId (e.g., using npcHandler:setTopic(playerId, 0) or the appropriate
clear-topic method) immediately after the random/else branches (before or
instead of leaving topic at 5) and then keep the
player:setStorageValue(Storage.Quest.U8_4.BloodBrothers.Mission03, 2) line
as-is.
In `@data-otservbr-global/npc/julius.lua`:
- Around line 348-361: The current completion branch only checks
BB.MapMarks_CastleEntrance and allows finishing Mission04 without sufficient
mapped places; change the guard to require both
player:getStorageValue(BB.MapMarks_CastleEntrance) == 1 and
(player:getStorageValue(BB.MapMarks) or 0) >= 5 before granting the reward and
setting BB.Mission04 to 2. Locate the block using BB.MapMarks_CastleEntrance,
BB.MapMarks, npcHandler:say and player:setStorageValue(BB.Mission04, 2) and
update the if-condition to include the minimum-mark check so the existing text
selection and removal logic remain executed only when both conditions are met.
In `@data-otservbr-global/scripts/actions/tools/skinning.lua`:
- Around line 168-180: The branch that awards quest dust returns early and never
marks the corpse as used; before returning true in the blood brother block
(inside the loop where missionItems is checked), call the same
corpse-marking/transform function used by the normal skinning path (the code
later in this file that transforms/removes the target after skinning) on target
so the corpse is transformed/consumed and cannot be reused for repeated dust
grants, then return true.
- Around line 140-155: The code currently removes the marble rock and grants
achievements even when player:addItem(_skin.newItem, 1) returns nil; change the
flow so you only call target:remove(),
toPosition:sendMagicEffect(CONST_ME_HITAREA) and award achievements
(player:addAchievement / player:addAchievementProgress) after confirming
gobletItem is non-nil. Locate the block using player:addItem, the local
gobletItem variable, the conditional checking _skin.newItem == 10429, and
target:remove(), and move those removal/effect/achievement actions inside the
gobletItem truthy branch so nothing is consumed or awarded on addItem failure.
In
`@data-otservbr-global/scripts/quests/blood_brothers_quest/actions_blood_crystal.lua`:
- Around line 6-9: The storage check/set is using the hardcoded key 58104
instead of the dynamic UID, so update the calls to player:getStorageValue(58104)
and player:setStorageValue(58104, 1) to use item.uid (i.e.,
player:getStorageValue(item.uid) and player:setStorageValue(item.uid, 1)) so
each reward entry in reward[item.uid] uses its own storage flag; ensure you only
change those two storage calls in the block where
player:addItem(reward[item.uid], 1) and player:sendTextMessage(...) are called.
In
`@data-otservbr-global/scripts/quests/blood_brothers_quest/actions_castle_book.lua`:
- Line 26: Replace the incorrect ordinal "III. 21th" with the correct form "III.
21st" in the story text constant (search for the literal string "III. 21th" in
actions_castle_book.lua and update it to "III. 21st"); ensure no other
occurrences of "21th" remain in the file.
- Line 77: Update the incorrect ordinal in the story text by replacing the
string "IV. 22th" with "IV. 22nd" in actions_castle_book.lua (locate the text
literal containing "IV. 22th" around the castle book action code and change it
to "IV. 22nd").
In
`@data-otservbr-global/scripts/quests/blood_brothers_quest/actions_ghost_tear.lua`:
- Line 24: The Game.getSpectators call is passing full deltas for both min/max
which shifts/expands the scan; change it to compute the midpoint between fromPos
and toPos and use half-width/half-height extents (abs(toPos.x - fromPos.x)/2 and
abs(toPos.y - fromPos.y)/2) as the min/max radii when calling Game.getSpectators
(i.e. call Game.getSpectators(midPos, false, false, halfX, halfX, halfY,
halfY)), and after retrieving spectators filter them with spectator:isPlayer()
(or similar) so only players are processed; update references to
Game.getSpectators, fromPos, toPos and the spectators variable accordingly.
In `@data-otservbr-global/scripts/quests/others/actions_fire_bug.lua`:
- Around line 56-77: Add an encounter lock check and set before spawning Boreth
to prevent retriggering while the fight is active: before the block that reads
Storage.Quest.U8_4.BloodBrothers.Plant1/Plant2/Plant3 and calls
Game.createMonster("Boreth", ...) set and require a new storage flag (e.g.,
Storage.Quest.U8_4.BloodBrothers.BorethLock) so the spawn code only runs if
BorethLock ~= 1, then set BorethLock = 1 immediately before creating the
monsters; move or delay resetting Plant1/Plant2/Plant3 to 0 until the encounter
ends (clear BorethLock and reset the plants from the Boreth death handler or a
timed checker), and reference the monster name "Boreth" to attach the cleanup
that clears the lock and resets the plant storages.
- Around line 146-152: The delayed cleanup currently removes the wrong object:
change the closure passed to addEvent to check and remove the created 'fire'
item (the result of Game.createItem(2118, ...)) instead of 'item' (the player's
tool). Locate the addEvent call in actions_fire_bug.lua and replace the
conditional to verify 'fire' exists and is an item (fire and fire:isItem()) and
then call fire:remove() inside the timeout callback so the spawned quest fire is
cleaned up, not the player's item.
In `@data-otservbr-global/world/otservbr-npc.xml`:
- Around line 6-7: The NPC definition for "A Wandering Soul" has mismatched
Z-levels: the parent <npc> has centerz="7" but the child npc entry has z="0",
causing it to spawn on the wrong floor; update the child npc element for name "A
Wandering Soul" to use z="7" (or remove the child z attribute so it inherits the
parent centerz) so the NPC spawns on the intended Z-level and keep
spawntime="60" unchanged.
---
Duplicate comments:
In
`@data-otservbr-global/scripts/quests/blood_brothers_quest/actions_lersatio_mirrors.lua`:
- Line 66: The Game.getSpectators call uses the same value for min and max x/y
ranges so it behaves like a radius from fromPos; compute explicit
left/right/up/down distances from fromPos to toPos instead and pass those to
Game.getSpectators. Concretely, calculate right = math.max(0, toPos.x -
fromPos.x), left = math.max(0, fromPos.x - toPos.x), down = math.max(0, toPos.y
- fromPos.y), up = math.max(0, fromPos.y - toPos.y) and call
Game.getSpectators(fromPos, false, false, right, left, down, up) (using the same
fromPos/toPos and spectators variable) so the function covers the rectangular
area between fromPos and toPos.
---
Nitpick comments:
In `@data-otservbr-global/npc/tamara.lua`:
- Around line 53-325: The file repeats many nearly identical blocks
(node1..node67 using keywordHandler:addKeyword with StdModule.say and
StdModule.learnSpell) which is error-prone; refactor by creating a data table
(array of spell entries with keys: trigger word, prompt text, spellName,
vocation, price, level, premium flag) and a small registrar function (e.g.,
registerSpell(entry)) that calls keywordHandler:addKeyword and adds the "yes"
child with StdModule.learnSpell; also convert the nodeLevels registrations
(nodeLevels:addChildKeyword) into a similar data-driven list so the level
messages are generated from a levels table—use existing symbols
keywordHandler:addKeyword, StdModule.learnSpell, StdModule.say, nodeLevels to
locate where to replace the repeated blocks.
In `@data-otservbr-global/npc/tamoril.lua`:
- Around line 113-374: The file contains many repetitive
keywordHandler:addKeyword + nodeX:addChildKeyword blocks (see node1..node63 and
nodeLevels) registering spells with StdModule.say and StdModule.learnSpell;
replace these repetitive blocks with a declarative table/array of spell entries
(fields: keywords, text, premium, spellName, vocation, price, level) and a small
loop that calls keywordHandler:addKeyword and then addChildKeyword for each
entry (use the same StdModule.say and StdModule.learnSpell handlers), and
likewise convert the nodeLevels child registrations into a data table iterated
to register level responses; keep existing symbols keywordHandler:addKeyword,
StdModule.say, StdModule.learnSpell and nodeLevels to locate where to swap in
the loop.
In `@data-otservbr-global/npc/zarak.lua`:
- Around line 48-180: The file has many repetitive keywordHandler:addKeyword +
nodeX:addChildKeyword calls (node1..node27) registering spells via
StdModule.learnSpell; refactor by creating a declarative spells table (each
entry: trigger, text, spellName, vocation, price, level, premium,
onlyFocus/premium flags) and iterate it to call keywordHandler:addKeyword and
addChildKeyword for each entry (preserve npcHandler, StdModule.say prompts and
StdModule.learnSpell parameters exactly). Do the same optionally to derive the
nodeLevels listings from the same table (keep existing nodeLevels
keywords/phrasing from nodeLevels variable) so metadata (spellName, price,
level, vocation, premium) stays single-source-of-truth and eliminates the
node1..node27 duplication.
In `@data-otservbr-global/scripts/creaturescripts/monster/boreth_death.lua`:
- Around line 7-18: Replace the nested for-loops and repeated
Tile(Position)/tile:getCreatures() calls with a single Game.getSpectators() call
that covers the rectangle fromPos..toPos; compute the center Position and x/y
ranges from fromPos and toPos, call Game.getSpectators(center, false, false,
rangeX, rangeY) and then iterate those spectators, keeping the existing checks
spectator:isMonster() and spectator:getName():lower() == "plaguethrower" and
calling spectator:remove() for matches; this removes creating many Tile objects
and improves performance while preserving the same logic around fromPos, toPos,
Position, spectator:isMonster(), spectator:getName(), and spectator:remove().
In
`@data-otservbr-global/scripts/creaturescripts/monster/vampire_bride_death.lua`:
- Around line 19-24: Remove the redundant spectator:isPlayer() conditional
inside the loop since Game.getSpectators was called with onlyPlayer = true;
update the loop over spectators (the variable spectators returned by
Game.getSpectators) to directly call spectator:teleportTo(teleportDest) and
teleportDest:sendMagicEffect(CONST_ME_TELEPORT) for each spectator, leaving
Game.getSpectators(...) and teleportDest references intact.
- Around line 1-2: The module-level tables vampireBrideKills and killCooldown
can be lost on script reload; replace them with persistent storage via
GlobalStorage (or equivalent server-wide storage) by introducing stable keys
(e.g., "vampireBrideKills" and "vampireBrideKillCooldowns") and helper functions
(get/set/clear) to read/write these maps instead of using the in-file tables,
update any references in functions that read or modify
vampireBrideKills/killCooldown to call those helpers, and add an
initialization/cleanup path that migrates existing in-memory state into
GlobalStorage on load and clears or resets entries on server restart as needed;
document behavior in comments so maintainers know reloads won't preserve
in-memory encounter state.
- Around line 26-27: Game.createMonster calls can return nil on failure, so
after calling Game.createMonster("Marziel", spawnPos) and
Game.createMonster("Vampire", Position(...)) check each return value and handle
nil: if spawn failed, log an error (including monster name and spawnPos), abort
or rollback the quest flow (so later logic doesn’t assume the monsters exist),
and optionally try alternative nearby positions or notify the player; reference
the Game.createMonster calls and the spawnPos / Position(...) used so you update
the spawning block to validate results and handle failures safely.
In
`@data-otservbr-global/scripts/quests/blood_brothers_quest/actions_lersatio_mirrors.lua`:
- Around line 68-73: The local variable playersTeleported is declared and
incremented in the loop over spectators but never read; remove the declaration
and the playersTeleported = playersTeleported + 1 line to avoid the unused
variable, leaving the loop to call spectator:teleportTo(teleportDestination) and
spectator:getPosition():sendMagicEffect(CONST_ME_TELEPORT) for each spectator;
if you prefer to keep a count for validation or logging, instead use that
counter (playersTeleported) after the loop to log or assert the expected number.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5c6498e5-2f97-489a-9dfc-0999da60b494
📒 Files selected for processing (66)
data-otservbr-global/lib/core/quests/catalog/052_blood_brothers_quest.luadata-otservbr-global/lib/core/quests/catalog/init.luadata-otservbr-global/lib/core/storages.luadata-otservbr-global/monster/quests/blood_brothers_quest/armenius.luadata-otservbr-global/monster/quests/blood_brothers_quest/arthei.luadata-otservbr-global/monster/quests/blood_brothers_quest/boreth.luadata-otservbr-global/monster/quests/blood_brothers_quest/lersatio.luadata-otservbr-global/monster/quests/blood_brothers_quest/marziel.luadata-otservbr-global/monster/quests/blood_brothers_quest/shadow_of_boreth.luadata-otservbr-global/monster/quests/blood_brothers_quest/shadow_of_lersatio.luadata-otservbr-global/monster/quests/blood_brothers_quest/shadow_of_marziel.luadata-otservbr-global/npc/a_fading_memory.luadata-otservbr-global/npc/a_wandering_soul.luadata-otservbr-global/npc/armenius.luadata-otservbr-global/npc/barry.luadata-otservbr-global/npc/bruce.luadata-otservbr-global/npc/captain_cookie.luadata-otservbr-global/npc/chephan.luadata-otservbr-global/npc/chuckles.luadata-otservbr-global/npc/dorbin.luadata-otservbr-global/npc/ethan.luadata-otservbr-global/npc/gree_dee.luadata-otservbr-global/npc/hal.luadata-otservbr-global/npc/harlow.luadata-otservbr-global/npc/harlow_vengoth.luadata-otservbr-global/npc/julius.luadata-otservbr-global/npc/karith.luadata-otservbr-global/npc/lisander.luadata-otservbr-global/npc/lorietta.luadata-otservbr-global/npc/maris.luadata-otservbr-global/npc/morpel.luadata-otservbr-global/npc/oiriz.luadata-otservbr-global/npc/oliver.luadata-otservbr-global/npc/palimuth.luadata-otservbr-global/npc/peter.luadata-otservbr-global/npc/pugwah.luadata-otservbr-global/npc/redward.luadata-otservbr-global/npc/reed.luadata-otservbr-global/npc/serafin.luadata-otservbr-global/npc/soilance.luadata-otservbr-global/npc/tamara.luadata-otservbr-global/npc/tamoril.luadata-otservbr-global/npc/timothy.luadata-otservbr-global/npc/tony.luadata-otservbr-global/npc/vincent.luadata-otservbr-global/npc/yalahari.luadata-otservbr-global/npc/zarak.luadata-otservbr-global/npc/znozel.luadata-otservbr-global/scripts/actions/other/baking.luadata-otservbr-global/scripts/actions/tools/skinning.luadata-otservbr-global/scripts/creaturescripts/monster/boreth_death.luadata-otservbr-global/scripts/creaturescripts/monster/vampire_bride_death.luadata-otservbr-global/scripts/quests/blood_brothers_quest/actions_blood_crystal.luadata-otservbr-global/scripts/quests/blood_brothers_quest/actions_blood_goblet.luadata-otservbr-global/scripts/quests/blood_brothers_quest/actions_castle_book.luadata-otservbr-global/scripts/quests/blood_brothers_quest/actions_ghost_tear.luadata-otservbr-global/scripts/quests/blood_brothers_quest/actions_julius_map.luadata-otservbr-global/scripts/quests/blood_brothers_quest/actions_lersatio_mirrors.luadata-otservbr-global/scripts/quests/blood_brothers_quest/movements_castle_entrace.luadata-otservbr-global/scripts/quests/blood_brothers_quest/movements_marziel_blood.luadata-otservbr-global/scripts/quests/blood_brothers_quest/movements_unlock_entrace.luadata-otservbr-global/scripts/quests/others/actions_fire_bug.luadata-otservbr-global/world/otservbr-npc.xmldata/items/items.xmldata/scripts/creaturescripts/player/castlemania.luadata/scripts/creaturescripts/player/login.lua
Comment on lines
+16
to
+19
| monster.bosstiary = { | ||
| bossRaceId = 479, | ||
| bossRace = RARITY_BANE, | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n 'bossRaceId\s*=\s*[0-9]+' data-otservbr-global/monster/quests/blood_brothers_quest \
| awk -F'[:=, ]+' '{
for (i=1; i<=NF; i++) {
if ($i=="bossRaceId") {
id=$(i+1);
print id " " $1 ":" $2;
}
}
}' \
| sort -n \
| awk '
{
id=$1; loc=$2;
count[id]++; refs[id]=refs[id] ? refs[id] ", " loc : loc;
}
END {
for (id in count) {
if (count[id] > 1) {
print "duplicate bossRaceId " id " -> " refs[id];
}
}
}'Repository: opentibiabr/canary
Length of output: 44
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== All bossRaceId assignments in quest set =="
rg -n --no-heading 'bossRaceId\s*=\s*[0-9]+' data-otservbr-global/monster/quests/blood_brothers_quest
echo
echo "== Literal 479 occurrences in quest set =="
rg -n --no-heading '479' data-otservbr-global/monster/quests/blood_brothers_quest
echo
echo "== Inspect shadow_of_boreth.lua around bosstiary =="
rg -n --no-heading 'bosstiary|bossRaceId|bossRace' data-otservbr-global/monster/quests/blood_brothers_quest/shadow_of_boreth.lua
echo
sed -n '1,80p' data-otservbr-global/monster/quests/blood_brothers_quest/shadow_of_boreth.lua
echo
echo "== Inspect boreth.lua around bosstiary/bossRaceId =="
sed -n '1,120p' data-otservbr-global/monster/quests/blood_brothers_quest/boreth.luaRepository: opentibiabr/canary
Length of output: 5482
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n --hidden --glob '!**/node_modules/**' 'bossRaceId' . || true
rg -n --hidden --glob '!**/node_modules/**' 'bosstiary' . || trueRepository: opentibiabr/canary
Length of output: 50374
Avoid reusing bosstiary.bossRaceId between primary bosses and their shadow summons (Blood Brothers quest)
shadow_of_boreth.luasetsmonster.bosstiary.bossRaceId = 479, which is the same ID asboreth.lua(bossRaceId = 479). The same pattern exists forshadow_of_marziel/marziel(481) andshadow_of_lersatio/lersatio(482).bossRaceIdis used by the server to look up/encode bosstiary data by race id, so sharing it across distinct monsters can merge/overwrite bosstiary tracking (cooldowns/kill stages/slot info).
Assign unique bosstiary.bossRaceId values for the shadow variants, or remove monster.bosstiary from summon-only shadow monster types.
🤖 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
`@data-otservbr-global/monster/quests/blood_brothers_quest/shadow_of_boreth.lua`
around lines 16 - 19, The shadow variants are reusing bosstiary.bossRaceId
values from their primary bosses (e.g., shadow_of_boreth.lua uses bossRaceId =
479 same as boreth.lua), which will collide in server bosstiary lookup; either
remove monster.bosstiary from the summon-only shadow monsters or assign each
shadow_* monster a unique bosstiary.bossRaceId distinct from the primary (same
approach applies to shadow_of_marziel/shadow_of_lersatio and their primaries),
update the monster.bosstiary table accordingly so that bossRaceId values are
unique per monster or omitted for summons.
| player:setStorageValue(Storage.Quest.U8_4.BloodBrothers.Mission03, 2) | ||
| else | ||
| npcHandler:say("Oh, the nerve. Sod off.", npc, creature) | ||
| elseif message:lower() == "alori mort" and player:getStorageValue(Storage.Quest.U8_4.BloodBrothers.Mission03) == 1 or player:getStorageValue(Storage.Quest.U8_4.BloodBrothers.Mission03) == 2 then |
There was a problem hiding this comment.
Parenthesize the alori mort mission check to avoid unintended branch entry.
Line 71 currently evaluates as (message == "alori mort" and Mission03 == 1) or Mission03 == 2.
That means any message can enter this branch once Mission03 is 2.
💡 Proposed fix
- elseif message:lower() == "alori mort" and player:getStorageValue(Storage.Quest.U8_4.BloodBrothers.Mission03) == 1 or player:getStorageValue(Storage.Quest.U8_4.BloodBrothers.Mission03) == 2 then
+ elseif message:lower() == "alori mort"
+ and (player:getStorageValue(Storage.Quest.U8_4.BloodBrothers.Mission03) == 1
+ or player:getStorageValue(Storage.Quest.U8_4.BloodBrothers.Mission03) == 2) then🤖 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 `@data-otservbr-global/npc/armenius.lua` at line 71, The conditional mixes
OR/AND causing the branch to run for any message when Mission03 == 2; update the
check in npc/armenius.lua so the message test and mission values are grouped
together: ensure the expression uses parentheses around (message:lower() ==
"alori mort" and
(player:getStorageValue(Storage.Quest.U8_4.BloodBrothers.Mission03) == 1 or
player:getStorageValue(Storage.Quest.U8_4.BloodBrothers.Mission03) == 2)) so
only the "alori mort" message triggers the branch when Mission03 is 1 or 2.
Comment on lines
+87
to
105
| elseif npcHandler:getTopic(playerId) == 5 then | ||
| local rand = math.random(3) | ||
| if rand == 1 then | ||
| npcHandler:setMessage(MESSAGE_WALKAWAY, "Oh, the nerve. Go to the rats which raised you.") | ||
| player:teleportTo(Position(32759, 31241, 9)) | ||
| player:getPosition():sendMagicEffect(CONST_ME_TELEPORT) | ||
| elseif rand == 2 then | ||
| npcHandler:setMessage(MESSAGE_WALKAWAY, "You dare say that again?! I'll send you straight to your grave!") | ||
| player:teleportTo(Position(32856, 31324, 8)) | ||
| player:getPosition():sendMagicEffect(CONST_ME_TELEPORT) | ||
| Game.createMonster("Armenius", Position(32857, 31324, 8)) | ||
| if not player:hasAchievement("His True Face") then | ||
| player:addAchievement("His True Face") | ||
| end | ||
| else | ||
| npcHandler:say("Oh, the nerve. Sod off.", npc, creature) | ||
| end | ||
| player:setStorageValue(Storage.Quest.U8_4.BloodBrothers.Mission03, 2) | ||
| end |
There was a problem hiding this comment.
Reset topic after the random outcome to prevent repeated retriggers.
After topic 5 resolves, the topic stays at 5. Combined with Mission03==2, this enables repeated teleport/spawn behavior on subsequent messages.
💡 Proposed fix
elseif npcHandler:getTopic(playerId) == 5 then
local rand = math.random(3)
if rand == 1 then
...
elseif rand == 2 then
...
else
npcHandler:say("Oh, the nerve. Sod off.", npc, creature)
end
player:setStorageValue(Storage.Quest.U8_4.BloodBrothers.Mission03, 2)
+ npcHandler:setTopic(playerId, 0)
end📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| elseif npcHandler:getTopic(playerId) == 5 then | |
| local rand = math.random(3) | |
| if rand == 1 then | |
| npcHandler:setMessage(MESSAGE_WALKAWAY, "Oh, the nerve. Go to the rats which raised you.") | |
| player:teleportTo(Position(32759, 31241, 9)) | |
| player:getPosition():sendMagicEffect(CONST_ME_TELEPORT) | |
| elseif rand == 2 then | |
| npcHandler:setMessage(MESSAGE_WALKAWAY, "You dare say that again?! I'll send you straight to your grave!") | |
| player:teleportTo(Position(32856, 31324, 8)) | |
| player:getPosition():sendMagicEffect(CONST_ME_TELEPORT) | |
| Game.createMonster("Armenius", Position(32857, 31324, 8)) | |
| if not player:hasAchievement("His True Face") then | |
| player:addAchievement("His True Face") | |
| end | |
| else | |
| npcHandler:say("Oh, the nerve. Sod off.", npc, creature) | |
| end | |
| player:setStorageValue(Storage.Quest.U8_4.BloodBrothers.Mission03, 2) | |
| end | |
| elseif npcHandler:getTopic(playerId) == 5 then | |
| local rand = math.random(3) | |
| if rand == 1 then | |
| npcHandler:setMessage(MESSAGE_WALKAWAY, "Oh, the nerve. Go to the rats which raised you.") | |
| player:teleportTo(Position(32759, 31241, 9)) | |
| player:getPosition():sendMagicEffect(CONST_ME_TELEPORT) | |
| elseif rand == 2 then | |
| npcHandler:setMessage(MESSAGE_WALKAWAY, "You dare say that again?! I'll send you straight to your grave!") | |
| player:teleportTo(Position(32856, 31324, 8)) | |
| player:getPosition():sendMagicEffect(CONST_ME_TELEPORT) | |
| Game.createMonster("Armenius", Position(32857, 31324, 8)) | |
| if not player:hasAchievement("His True Face") then | |
| player:addAchievement("His True Face") | |
| end | |
| else | |
| npcHandler:say("Oh, the nerve. Sod off.", npc, creature) | |
| end | |
| player:setStorageValue(Storage.Quest.U8_4.BloodBrothers.Mission03, 2) | |
| npcHandler:setTopic(playerId, 0) | |
| end |
🤖 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 `@data-otservbr-global/npc/armenius.lua` around lines 87 - 105, After handling
npcHandler:getTopic(playerId) == 5 and performing the random branch actions (the
MESSAGE_WALKAWAY branches, teleport/spawn and achievement code), clear the NPC
conversation topic for this player so the same topic won't retrigger; call the
NPC API to reset the topic for playerId (e.g., using
npcHandler:setTopic(playerId, 0) or the appropriate clear-topic method)
immediately after the random/else branches (before or instead of leaving topic
at 5) and then keep the
player:setStorageValue(Storage.Quest.U8_4.BloodBrothers.Mission03, 2) line
as-is.
Comment on lines
+348
to
+361
| if player:getStorageValue(BB.MapMarks_CastleEntrance) == 1 then | ||
| local marks = player:getStorageValue(BB.MapMarks) or 0 | ||
| local text = "five" | ||
| if marks == 6 then | ||
| text = "seven" | ||
| elseif marks == 7 then -- entrace does not count | ||
| text = "eight" | ||
| end | ||
| npcHandler:say("Well done, you even marked " .. text .. " places! I'll grant you a little bonus for that. Come back later and ask me about your next mission. I have to think.", npc, creature) | ||
| player:setStorageValue(BB.Mission04, 2) | ||
| player:removeItem(8200, 1) | ||
| npcHandler:setTopic(playerId, 0) | ||
| else | ||
| npcHandler:say("No, no, I was asking for one of the names.", npc, creature) | ||
| npcHandler:say("You haven't mapped enough places yet. Come back once you have found at least five remarkable places, including the castle.", npc, creature) |
There was a problem hiding this comment.
Enforce the minimum map mark count before completing Mission 04.
Line 348 only checks MapMarks_CastleEntrance == 1, so players can complete this step without the required total mapped locations.
💡 Proposed fix
- if player:getStorageValue(BB.MapMarks_CastleEntrance) == 1 then
- local marks = player:getStorageValue(BB.MapMarks) or 0
+ local marks = math.max(player:getStorageValue(BB.MapMarks), 0)
+ if player:getStorageValue(BB.MapMarks_CastleEntrance) == 1 and marks >= 5 then
local text = "five"
if marks == 6 then
text = "seven"
elseif marks == 7 then -- entrace does not count
text = "eight"
end
...
else
npcHandler:say("You haven't mapped enough places yet. Come back once you have found at least five remarkable places, including the castle.", npc, creature)
npcHandler:setTopic(playerId, 0)
end📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if player:getStorageValue(BB.MapMarks_CastleEntrance) == 1 then | |
| local marks = player:getStorageValue(BB.MapMarks) or 0 | |
| local text = "five" | |
| if marks == 6 then | |
| text = "seven" | |
| elseif marks == 7 then -- entrace does not count | |
| text = "eight" | |
| end | |
| npcHandler:say("Well done, you even marked " .. text .. " places! I'll grant you a little bonus for that. Come back later and ask me about your next mission. I have to think.", npc, creature) | |
| player:setStorageValue(BB.Mission04, 2) | |
| player:removeItem(8200, 1) | |
| npcHandler:setTopic(playerId, 0) | |
| else | |
| npcHandler:say("No, no, I was asking for one of the names.", npc, creature) | |
| npcHandler:say("You haven't mapped enough places yet. Come back once you have found at least five remarkable places, including the castle.", npc, creature) | |
| local marks = player:getStorageValue(BB.MapMarks) or 0 | |
| if player:getStorageValue(BB.MapMarks_CastleEntrance) == 1 and marks >= 5 then | |
| local text = "five" | |
| if marks == 6 then | |
| text = "seven" | |
| elseif marks == 7 then -- entrace does not count | |
| text = "eight" | |
| end | |
| npcHandler:say("Well done, you even marked " .. text .. " places! I'll grant you a little bonus for that. Come back later and ask me about your next mission. I have to think.", npc, creature) | |
| player:setStorageValue(BB.Mission04, 2) | |
| player:removeItem(8200, 1) | |
| npcHandler:setTopic(playerId, 0) | |
| else | |
| npcHandler:say("You haven't mapped enough places yet. Come back once you have found at least five remarkable places, including the castle.", npc, creature) | |
| npcHandler:setTopic(playerId, 0) | |
| end |
🤖 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 `@data-otservbr-global/npc/julius.lua` around lines 348 - 361, The current
completion branch only checks BB.MapMarks_CastleEntrance and allows finishing
Mission04 without sufficient mapped places; change the guard to require both
player:getStorageValue(BB.MapMarks_CastleEntrance) == 1 and
(player:getStorageValue(BB.MapMarks) or 0) >= 5 before granting the reward and
setting BB.Mission04 to 2. Locate the block using BB.MapMarks_CastleEntrance,
BB.MapMarks, npcHandler:say and player:setStorageValue(BB.Mission04, 2) and
update the if-condition to include the minimum-mark check so the existing text
selection and removal logic remain executed only when both conditions are met.
| text = [[<this is a ripped page, probably from a diary> | ||
| IV. 20th | ||
| Since we've moved into this house, we barely had any contact to the other citizens of Yalahar. However this has changed during the last week. People are getting suspicious of Arthei - some saw him after the explosion and know that his sudden recovery can't have happened in a natural way. And they are starting to talk... what makes matters worse is that some murders have occurred since Arthei's recovery. I don't know any details or what exactly was the cause for their death, but I can't stop thinking that Arthei might have something to do with it. His nightly excursions... the state he is in... and it seems that I'm not the only one who thinks that way. As I said... people are starting to talk... | ||
| IV. 22th |
There was a problem hiding this comment.
Fix ordinal number: "22th" should be "22nd".
The date "IV. 22th" uses an incorrect ordinal suffix in the story text.
📝 Proposed fix
-IV. 22th
+IV. 22nd📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| IV. 22th | |
| IV. 22nd |
🤖 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
`@data-otservbr-global/scripts/quests/blood_brothers_quest/actions_castle_book.lua`
at line 77, Update the incorrect ordinal in the story text by replacing the
string "IV. 22th" with "IV. 22nd" in actions_castle_book.lua (locate the text
literal containing "IV. 22th" around the castle book action code and change it
to "IV. 22nd").
| local toPos = Position(32957, 31447, 2) | ||
| local teleportDestination = Position(32953, 31444, 1) | ||
|
|
||
| local spectators = Game.getSpectators(fromPos, false, false, toPos.x - fromPos.x, toPos.x - fromPos.x, toPos.y - fromPos.y, toPos.y - fromPos.y) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for other Game.getSpectators usage patterns to verify the correct API signature
rg -nP 'Game\.getSpectators\s*\(' --type=lua -C2Repository: opentibiabr/canary
Length of output: 50374
🏁 Script executed:
#!/bin/bash
set -euo pipefail
repo_root="."
echo "== Inspect actions_ghost_tear.lua around spectator call =="
target="data-otservbr-global/scripts/quests/blood_brothers_quest/actions_ghost_tear.lua"
nl -ba "$target" | sed -n '1,120p'
echo
echo "== Inspect actions_fire_bug.lua around similar spectator call =="
target2="data-otservbr-global/scripts/quests/others/actions_fire_bug.lua"
nl -ba "$target2" | sed -n '1,140p'
echo
echo "== Find other identical min/max range patterns (fromPos/toPos diffs) =="
rg -n --type=lua "Game\.getSpectators\(\s*fromPos\s*,\s*false\s*,\s*false\s*,\s*toPos\.x\s*-\s*fromPos\.x\s*,\s*toPos\.x\s*-\s*fromPos\.x\s*,\s*toPos\.y\s*-\s*fromPos\.y\s*,\s*toPos\.y\s*-\s*fromPos\.y" .
echo
echo "== Find other uses that compute ranges from toPos-fromPos for getSpectators =="
rg -n --type=lua "Game\.getSpectators\(\s*fromPos\b.*toPos\.x\s*-\s*fromPos\.x.*toPos\.y\s*-\s*fromPos\.y" .Repository: opentibiabr/canary
Length of output: 182
🏁 Script executed:
#!/bin/bash
set -euo pipefail
awk_show(){
# $1=file $2=start $3=end
awk -v s="$2" -v e="$3" 'NR>=s && NR<=e {printf "%6d\t%s\n", NR, $0}' "$1"
}
echo "== actions_ghost_tear.lua (around spectator call) =="
target="data-otservbr-global/scripts/quests/blood_brothers_quest/actions_ghost_tear.lua"
awk_show "$target" 1 120
echo
echo "== actions_fire_bug.lua (around similar spectator call) =="
target2="data-otservbr-global/scripts/quests/others/actions_fire_bug.lua"
awk_show "$target2" 1 120
echo
echo "== Find overloads that pass an explicit toPos as extra arg =="
rg -n --type=lua 'Game\.getSpectators\s*\(.*toPos' data-otservbr-global/scripts data-otservbr-global/lib data-otservbr-global/lib/quests || true
echo
echo "== Find getSpectators usage computing ranges from toPos/fromPos diffs (more specifically) =="
rg -n --type=lua 'Game\.getSpectators\(\s*fromPos\s*,[^)]*toPos\.x\s*-\s*fromPos\.x\s*,\s*toPos\.x\s*-\s*fromPos\.x\s*,\s*toPos\.y\s*-\s*fromPos\.y\s*,\s*toPos\.y\s*-\s*fromPos\.y' data-otservbr-global/scripts || trueRepository: opentibiabr/canary
Length of output: 9282
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect spectators range conversion helper
target="data-otservbr-global/lib/functions/spectators.lua"
if [ -f "$target" ]; then
echo "== spectators.lua around convertPosToRange and getSpectators call =="
awk 'NR>=1 && NR<=260 {print}' "$target" | sed -n '120,200p'
fi
echo
echo "== actions_lersatio_mirrors.lua around spectator call =="
target2="data-otservbr-global/scripts/quests/blood_brothers_quest/actions_lersatio_mirrors.lua"
awk 'NR>=1 && NR<=140 {print}' "$target2" | sed -n '40,100p'Repository: opentibiabr/canary
Length of output: 1660
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== spectators.lua: search for convertPosToRange and getSpectators usage =="
target="data-otservbr-global/lib/functions/spectators.lua"
rg -n --fixed-strings "convertPosToRange" "$target"
rg -n --fixed-strings "Game.getSpectators" "$target"
rg -n "getSpectators\\(" "$target"
echo
echo "== spectators.lua: print around convertPosToRange/Range conversion =="
awk 'NR>=90 && NR<=190 {print NR ":" $0}' "$target" | sed -n '1,120p'
echo
echo "== mission11_payback_time_lever.lua: inspect around startAreaPosition getSpectators =="
target2="data-otservbr-global/scripts/quests/wrath_of_the_emperor/actions_mission11_payback_time_lever.lua"
awk 'NR>=1 && NR<=90 {print NR ":" $0}' "$target2" | sed -n '1,90p'Repository: opentibiabr/canary
Length of output: 245
🏁 Script executed:
#!/bin/bash
set -euo pipefail
target="data/libs/functions/spectators.lua"
if [ ! -f "$target" ]; then
echo "Missing $target"
exit 1
fi
echo "== spectators.lua (data/libs/functions/spectators.lua) around convertPosToRange =="
awk_show(){
awk -v s="$2" -v e="$3" 'NR>=s && NR<=e {printf "%6d\t%s\n", NR, $0}' "$1"
}
awk_show "$target" 1 260
echo
echo "== Inspect getSpectators call in spectators.lua (likely around line ~140) =="
awk_show "$target" 120 170
echo
echo "== Inspect other helper patterns: data/libs/compat/compat.lua getSpectators wrapper =="
target2="data/libs/compat/compat.lua"
if [ -f "$target2" ]; then
awk_show "$target2" 1460 1505
else
echo "Missing $target2"
fiRepository: opentibiabr/canary
Length of output: 7784
Fix spectator range calculation in actions_ghost_tear.lua
The Game.getSpectators call uses the full deltas (toPos.x - fromPos.x, toPos.y - fromPos.y) for both min/max while keeping fromPos as the reference position:
local spectators = Game.getSpectators(fromPos, false, false, toPos.x - fromPos.x, toPos.x - fromPos.x, toPos.y - fromPos.y, toPos.y - fromPos.y)This doesn’t match the rectangle-scanning semantics used elsewhere in this codebase (e.g., data/libs/functions/spectators.lua computes (to-from)/2 and queries around the midpoint). As a result, the scanned area is shifted/expanded relative to the intended fromPos→toPos rectangle, and there’s no filtering beyond spectator:isPlayer().
🤖 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
`@data-otservbr-global/scripts/quests/blood_brothers_quest/actions_ghost_tear.lua`
at line 24, The Game.getSpectators call is passing full deltas for both min/max
which shifts/expands the scan; change it to compute the midpoint between fromPos
and toPos and use half-width/half-height extents (abs(toPos.x - fromPos.x)/2 and
abs(toPos.y - fromPos.y)/2) as the min/max radii when calling Game.getSpectators
(i.e. call Game.getSpectators(midPos, false, false, halfX, halfX, halfY,
halfY)), and after retrieving spectators filter them with spectator:isPlayer()
(or similar) so only players are processed; update references to
Game.getSpectators, fromPos, toPos and the spectators variable accordingly.
Comment on lines
+56
to
+77
| if player:getStorageValue(Storage.Quest.U8_4.BloodBrothers.Plant1) == 1 and player:getStorageValue(Storage.Quest.U8_4.BloodBrothers.Plant2) == 1 and player:getStorageValue(Storage.Quest.U8_4.BloodBrothers.Plant3) == 1 then | ||
| local fromPos = Position(32936, 31474, 2) | ||
| local toPos = Position(32944, 31482, 2) | ||
| local teleportDestination = Position(32940, 31478, 1) | ||
|
|
||
| local spectators = Game.getSpectators(fromPos, false, false, toPos.x - fromPos.x, toPos.x - fromPos.x, toPos.y - fromPos.y, toPos.y - fromPos.y) | ||
|
|
||
| for _, spectator in ipairs(spectators) do | ||
| if spectator:isPlayer() then | ||
| spectator:teleportTo(teleportDestination) | ||
| spectator:getPosition():sendMagicEffect(CONST_ME_TELEPORT) | ||
| end | ||
| end | ||
|
|
||
| Game.createMonster("Boreth", Position(32940, 31476, 1)) | ||
| Game.createMonster("plaguethrower", Position(32938, 31476, 1)) | ||
| Game.createMonster("plaguethrower", Position(32942, 31476, 1)) | ||
| player:setStorageValue(Storage.Quest.U8_4.BloodBrothers.Plant1, 0) | ||
| player:setStorageValue(Storage.Quest.U8_4.BloodBrothers.Plant2, 0) | ||
| player:setStorageValue(Storage.Quest.U8_4.BloodBrothers.Plant3, 0) | ||
| player:say("I WARNED YOU.", TALKTYPE_MONSTER_SAY) | ||
| end |
There was a problem hiding this comment.
Add an encounter lock before spawning Boreth.
Resetting Plant1/Plant2/Plant3 back to 0 immediately after Lines 70-75 means the same player can retrigger the three plants and spawn another Boreth/plaguethrower set before the current fight is resolved.
🤖 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 `@data-otservbr-global/scripts/quests/others/actions_fire_bug.lua` around lines
56 - 77, Add an encounter lock check and set before spawning Boreth to prevent
retriggering while the fight is active: before the block that reads
Storage.Quest.U8_4.BloodBrothers.Plant1/Plant2/Plant3 and calls
Game.createMonster("Boreth", ...) set and require a new storage flag (e.g.,
Storage.Quest.U8_4.BloodBrothers.BorethLock) so the spawn code only runs if
BorethLock ~= 1, then set BorethLock = 1 immediately before creating the
monsters; move or delay resetting Plant1/Plant2/Plant3 to 0 until the encounter
ends (clear BorethLock and reset the plants from the Boreth death handler or a
timed checker), and reference the monster name "Boreth" to attach the cleanup
that clears the lock and resets the plant storages.
Comment on lines
+146
to
+152
| local fire = Game.createItem(2118, 1, Position(32893, 32012, 6)) | ||
| player:setStorageValue(Storage.Quest.U8_1.SecretService.TBIMission01, 2) | ||
| addEvent(function() | ||
| if fire and item:isItem() then | ||
| item:remove() | ||
| end | ||
| end, 7 * 60 * 1000) |
There was a problem hiding this comment.
The delayed cleanup removes the wrong item.
Line 150 removes item, which is the fire bug the player used, not the fire created on Line 146. That can delete the player's tool while leaving the spawned quest fire untouched.
Suggested fix
- addEvent(function()
- if fire and item:isItem() then
- item:remove()
- end
- end, 7 * 60 * 1000)
+ addEvent(function()
+ if fire and fire:isItem() then
+ fire:remove()
+ end
+ end, 7 * 60 * 1000)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| local fire = Game.createItem(2118, 1, Position(32893, 32012, 6)) | |
| player:setStorageValue(Storage.Quest.U8_1.SecretService.TBIMission01, 2) | |
| addEvent(function() | |
| if fire and item:isItem() then | |
| item:remove() | |
| end | |
| end, 7 * 60 * 1000) | |
| local fire = Game.createItem(2118, 1, Position(32893, 32012, 6)) | |
| player:setStorageValue(Storage.Quest.U8_1.SecretService.TBIMission01, 2) | |
| addEvent(function() | |
| if fire and fire:isItem() then | |
| fire:remove() | |
| end | |
| end, 7 * 60 * 1000) |
🤖 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 `@data-otservbr-global/scripts/quests/others/actions_fire_bug.lua` around lines
146 - 152, The delayed cleanup currently removes the wrong object: change the
closure passed to addEvent to check and remove the created 'fire' item (the
result of Game.createItem(2118, ...)) instead of 'item' (the player's tool).
Locate the addEvent call in actions_fire_bug.lua and replace the conditional to
verify 'fire' exists and is an item (fire and fire:isItem()) and then call
fire:remove() inside the timeout callback so the spawned quest fire is cleaned
up, not the player's item.
Comment on lines
+6
to
+7
| <npc centerx="32914" centery="31490" centerz="7" radius="1"> | ||
| <npc name="A Wandering Soul" x="0" y="0" z="0" spawntime="60" /> |
There was a problem hiding this comment.
Spawn Z-level looks inconsistent for “A Wandering Soul”.
Line 6 sets centerz="7" but Line 7 sets NPC z="0". This likely places/spawns the NPC on the wrong floor.
💡 Proposed fix
- <npc name="A Wandering Soul" x="0" y="0" z="0" spawntime="60" />
+ <npc name="A Wandering Soul" x="0" y="0" z="7" spawntime="60" />📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <npc centerx="32914" centery="31490" centerz="7" radius="1"> | |
| <npc name="A Wandering Soul" x="0" y="0" z="0" spawntime="60" /> | |
| <npc centerx="32914" centery="31490" centerz="7" radius="1"> | |
| <npc name="A Wandering Soul" x="0" y="0" z="7" spawntime="60" /> |
🤖 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 `@data-otservbr-global/world/otservbr-npc.xml` around lines 6 - 7, The NPC
definition for "A Wandering Soul" has mismatched Z-levels: the parent <npc> has
centerz="7" but the child npc entry has z="0", causing it to spawn on the wrong
floor; update the child npc element for name "A Wandering Soul" to use z="7" (or
remove the child z attribute so it inherits the parent centerz) so the NPC
spawns on the intended Z-level and keep spawntime="60" unchanged.
Member
|
Blocking issue: the CI failure is caused by the quest catalog entry not matching the added file.
Also, simply renaming the file may not be enough because |
Contributor
Author
Fixed. |
Sorry I deleted things on my custom server, forgot to fix vincent here.
|
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.
WARNING: Don't merge this pull request Until Majesty upload the map Update I sent to him or Quest will be broken.
Complete Blood Brothers Quest + Questlog
Every detail has been created, achievements, dialogs and more.
Everything has been debuged and tested at https://tibiatales.com/