feat: retro bundle#3980
Conversation
Play canary server with oldschool 7.6 rules. 1.- Configurable retro features at config.lua 2.- Walk thru players. 3.- Lure creatures. 4.- Block respawn. 5.- Offline Training. 6.- Market. Notes: For an extra oldschool feature Learning Spells please merge: opentibiabr#3971 Everything has been debuged and tested at https://tibiatales.com using canary/tfs crystal server distribution.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds new config keys and defaults, applies them to gate market, offline training, spawn and movement logic, and introduces an advanced PartyProtection creature event registered at player login. ChangesFeature flags and gating
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![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 introduces several new configuration options to toggle or modify game features such as monster luring, respawn blocking, offline training, player walkthrough, and market access. The code review identifies a critical bug in the respawn blocking logic where a newly defined variable is ignored, an opportunity to optimize creature iteration on tiles, and a request to maintain consistent tab indentation. Additionally, the reviewer recommends separating unrelated enum alphabetization into a distinct pull request to improve clarity.
I am having trouble creating individual review comments. Click here to see my feedback.
src/creatures/monsters/spawns/spawn_monster.cpp (293-299)
The variable spawnBlockedByPlayer is calculated based on the BLOCK_RESPAWN configuration but is not used in the subsequent if condition. The condition still uses the original hardcoded logic, making the new configuration ineffective.
bool spawnBlockedByPlayer = false;
if (g_configManager().getBoolean(BLOCK_RESPAWN)) {
spawnBlockedByPlayer = findPlayer(sb.pos);
} else {
spawnBlockedByPlayer = mType->info.isBlockable && findPlayer(sb.pos);
}
if (!mType->canSpawn(sb.pos) || spawnBlockedByPlayer) {src/items/tile.cpp (690-700)
Iterating over the creatures vector twice is inefficient. The check for WALK_THROUGH_PLAYERS can be integrated into the existing loop to improve performance and maintainability.
for (const auto &tileCreature : *creatures) {
if (!g_configManager().getBoolean(WALK_THROUGH_PLAYERS) && tileCreature->getPlayer()) {
return RETURNVALUE_NOTENOUGHROOM;
}
if (!player->canWalkthrough(tileCreature)) {
return RETURNVALUE_NOTPOSSIBLE;
}
}data/scripts/actions/objects/skill_trainer.lua (1-31)
The indentation in this file has been changed from tabs to spaces. To maintain consistency with the rest of the project and keep the diff clean, please use tabs for indentation.
local setting = {
[16198] = SKILL_SWORD,
[16199] = SKILL_AXE,
[16200] = SKILL_CLUB,
[16201] = SKILL_DISTANCE,
[16202] = SKILL_MAGLEVEL,
[50296] = SKILL_FIST,
}
local skillTrainer = Action()
function skillTrainer.onUse(player, item, fromPosition, target, toPosition, isHotkey)
if not configManager.getBoolean(configKeys.ENABLE_OFFLINE_TRAINING) then
player:sendTextMessage(MESSAGE_EVENT_ADVANCE, "Offline training is currently disabled.")
return true
end
local skill = setting[item:getId()]
if not player:isPremium() and player:getSkillLevel(skill) > 50 then
player:sendCancelMessage(RETURNVALUE_YOUNEEDPREMIUMACCOUNT)
return true
end
if player:isPzLocked() then
return false
end
player:setOfflineTrainingSkill(skill)
player:remove(false)
return true
end
for index, value in pairs(setting) do
skillTrainer:id(index)
end
skillTrainer:register()
src/config/config_enums.hpp (20-22)
This PR includes reorganization (alphabetization) of existing enum members (like AMPLIFICATION_CHANCE_FORMULA_A) that are unrelated to the 'retro bundle' feature. Mixing refactoring with feature changes creates unnecessary noise in the diff. It is recommended to keep such refactoring in a separate PR.
|
![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/config/configmanager.cpp`:
- Line 82: The configured fallback defaults for creature lure and respawn are
inverted: change the boolean defaults passed to loadBoolConfig for
ALLOW_LURE_CREATURES ("allowLureCreatures") and ALLOW_BLOCK_RESPAWN
("blockRespawn") from true to false so missing keys follow config.lua.dist;
locate calls to loadBoolConfig using the symbols ALLOW_LURE_CREATURES and
ALLOW_BLOCK_RESPAWN in configmanager.cpp and update their third-argument default
values accordingly.
In `@src/creatures/monsters/spawns/spawn_monster.cpp`:
- Around line 293-300: The guard incorrectly recomputes the player-block
condition instead of using the computed spawnBlockedByPlayer; replace the direct
findPlayer/mType->info.isBlockable check in the if that sets sb.lastSpawn with
spawnBlockedByPlayer so the BLOCK_RESPAWN toggle (computed via
g_configManager().getBoolean(BLOCK_RESPAWN)) is honored, keeping the other part
of the condition mType->canSpawn(sb.pos) unchanged and still calling
OTSYS_TIME() when the combined condition fails.
In `@src/items/tile.cpp`:
- Around line 690-694: The current loop in tile.cpp unconditionally treats any
player in creatures as blocking when WALK_THROUGH_PLAYERS is false; change the
check inside the loop that currently uses tileCreature->getPlayer() to ignore
players in ghost mode by additionally verifying they are not ghosts (e.g.,
require !tileCreature->getPlayer()->isGhost() or check
!tileCreature->hasCondition(CONDITION_GHOST) depending on available API) before
returning RETURNVALUE_NOTENOUGHROOM so ghost-mode players do not hard-block
movement.
🪄 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: 88b9142b-0708-41cc-8c3c-7d7fbfedb545
📒 Files selected for processing (10)
config.lua.distdata/scripts/actions/objects/market.luadata/scripts/actions/objects/skill_trainer.luasrc/config/config_enums.hppsrc/config/configmanager.cppsrc/creatures/monsters/monster.cppsrc/creatures/monsters/spawns/spawn_monster.cppsrc/creatures/players/player.cppsrc/items/tile.cppsrc/server/network/protocol/protocolgame.cpp
| if (!g_configManager().getBoolean(WALK_THROUGH_PLAYERS)) { | ||
| for (const auto &tileCreature : *creatures) { | ||
| if (tileCreature->getPlayer()) { | ||
| return RETURNVALUE_NOTENOUGHROOM; | ||
| } |
There was a problem hiding this comment.
Exclude ghost-mode players from hard block
Line 692 blocks on any player, including ghost-mode players. That can make movement fail unexpectedly when WALK_THROUGH_PLAYERS is disabled.
Suggested fix
- for (const auto &tileCreature : *creatures) {
- if (tileCreature->getPlayer()) {
+ for (const auto &tileCreature : *creatures) {
+ if (tileCreature->getPlayer() && !tileCreature->isInGhostMode()) {
return RETURNVALUE_NOTENOUGHROOM;
}
}🧰 Tools
🪛 GitHub Check: Fast Checks / run-checks
[warning] 692-692:
[cppcheck] reported by reviewdog 🐶
Consider using std::any_of algorithm instead of a raw loop.
Raw Output:
src/items/tile.cpp:692:I:Consider using std::any_of algorithm instead of a raw loop.
🪛 GitHub Check: SonarCloud Code Analysis
[failure] 690-690: Refactor this code to not nest more than 3 if|for|do|while|switch statements.
🤖 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/items/tile.cpp` around lines 690 - 694, The current loop in tile.cpp
unconditionally treats any player in creatures as blocking when
WALK_THROUGH_PLAYERS is false; change the check inside the loop that currently
uses tileCreature->getPlayer() to ignore players in ghost mode by additionally
verifying they are not ghosts (e.g., require
!tileCreature->getPlayer()->isGhost() or check
!tileCreature->hasCondition(CONDITION_GHOST) depending on available API) before
returning RETURNVALUE_NOTENOUGHROOM so ghost-mode players do not hard-block
movement.
for retro gameplay with smart party protection and summons
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
data/scripts/creaturescripts/player/login.lua (1)
166-166: ⚡ Quick winConditionally register
PartyProtectiononly when the feature flag is enabled.This avoids unnecessary
onHealthChangecallback overhead whenADVANCED_PARTY_PROTECTIONis off.Suggested change
- player:registerEvent("PartyProtection") + if configManager.getBoolean(configKeys.ADVANCED_PARTY_PROTECTION) then + player:registerEvent("PartyProtection") + 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/scripts/creaturescripts/player/login.lua` at line 166, Wrap the player:registerEvent("PartyProtection") call so it only runs when the ADVANCED_PARTY_PROTECTION feature flag is enabled; locate the registration in login.lua and gate it behind a check of ADVANCED_PARTY_PROTECTION (or the project's feature flag accessor) to avoid registering the PartyProtection onHealthChange callback when the feature is off.
🤖 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/config/configmanager.cpp`:
- Line 80: The default fallback for advancedPartyProtection is inconsistent with
the distributed config: in configmanager.cpp change the loadBoolConfig call for
ADVANCED_PARTY_PROTECTION (currently using true) to use false so missing keys
default to the same value as config.lua.dist; update the call to
loadBoolConfig(L, ADVANCED_PARTY_PROTECTION, "advancedPartyProtection", false)
to align behavior.
---
Nitpick comments:
In `@data/scripts/creaturescripts/player/login.lua`:
- Line 166: Wrap the player:registerEvent("PartyProtection") call so it only
runs when the ADVANCED_PARTY_PROTECTION feature flag is enabled; locate the
registration in login.lua and gate it behind a check of
ADVANCED_PARTY_PROTECTION (or the project's feature flag accessor) to avoid
registering the PartyProtection onHealthChange callback when the feature is off.
🪄 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: 332e84ea-5ea0-47b4-b155-e9d2ea98b695
📒 Files selected for processing (5)
config.lua.distdata/scripts/creaturescripts/player/advanced_party_protection.luadata/scripts/creaturescripts/player/login.luasrc/config/config_enums.hppsrc/config/configmanager.cpp
| loadStringConfig(L, MYSQL_USER, "mysqlUser", "root"); | ||
| } | ||
|
|
||
| loadBoolConfig(L, ADVANCED_PARTY_PROTECTION, "advancedPartyProtection", true); |
There was a problem hiding this comment.
This fallback does not match config.lua.dist, where advancedPartyProtection is documented as disabled by default. Existing servers that update without adding the new config key would silently enable advanced party protection.
| loadBoolConfig(L, ADVANCED_PARTY_PROTECTION, "advancedPartyProtection", true); | |
| loadBoolConfig(L, ADVANCED_PARTY_PROTECTION, "advancedPartyProtection", false); |
There was a problem hiding this comment.
This will disable the code even if true at config.lua, leave as it is. Make new users update config.lua
| loadBoolConfig(L, ADVANCED_PARTY_PROTECTION, "advancedPartyProtection", true); | ||
| loadBoolConfig(L, AIMBOT_HOTKEY_ENABLED, "hotkeyAimbotEnabled", true); | ||
| loadBoolConfig(L, ALLOW_CHANGEOUTFIT, "allowChangeOutfit", true); | ||
| loadBoolConfig(L, ALLOW_LURE_CREATURES, "allowLureCreatures", true); |
There was a problem hiding this comment.
This fallback does not match config.lua.dist, where allowLureCreatures is disabled by default. Existing servers missing the new key would silently enable unlimited lure behavior.
| loadBoolConfig(L, ALLOW_LURE_CREATURES, "allowLureCreatures", true); | |
| loadBoolConfig(L, ALLOW_LURE_CREATURES, "allowLureCreatures", false); |
| loadBoolConfig(L, AUTOBANK, "autoBank", false); | ||
| loadBoolConfig(L, AUTOLOOT, "autoLoot", false); | ||
| loadBoolConfig(L, BOOSTED_BOSS_SLOT, "boostedBossSlot", true); | ||
| loadBoolConfig(L, BLOCK_RESPAWN, "blockRespawn", true); |
There was a problem hiding this comment.
This fallback does not match config.lua.dist, where blockRespawn is disabled by default. Existing servers missing the new key would silently change respawn blocking behavior.
| loadBoolConfig(L, BLOCK_RESPAWN, "blockRespawn", true); | |
| loadBoolConfig(L, BLOCK_RESPAWN, "blockRespawn", false); |
Now respect those who are set nonblock
It's better this way, so people can configure resapwn despite the config.
|
2 participants
Play canary server with oldschool 7.6 rules.
1.- Configurable retro features at config.lua
2.- Walk thru players.
3.- Lure creatures.
4.- Block respawn.
5.- Offline Training.
6.- Market.
7.- Advanced Party Protection, useful in retroPvP gameplay.
Notes:
1.- For an extra oldschool feature Learning Spells please merge: #3971
2.- I didn't create the level requirements lift to armor and weapons because due the nature of the new tibia pvp, map and respawns it would be an invitation to break the game and degradate gameplay.
Everything has been debuged and tested at https://tibiatales.com/