Fix Compile Issue and Update Trainers/SQL - #39
Conversation
…ctioneers may be purchased.
… allows the extra trainers to be added for free if they are missing.
…e in case it does not exist yet.
|
Can you explain why the Innkeeper is useless? It will be used to set Hearthstone location right? that seems useful Edit: Well okey you can teleport there with chat command anyway, so indeed useless. |
Co-authored-by: Dedmen Miller <dedmen@users.noreply.github.com>
Co-authored-by: Dedmen Miller <dedmen@users.noreply.github.com>
Co-authored-by: Dedmen Miller <dedmen@users.noreply.github.com>
Co-authored-by: Dedmen Miller <dedmen@users.noreply.github.com>
Co-authored-by: Dedmen Miller <dedmen@users.noreply.github.com>
Co-authored-by: Dedmen Miller <dedmen@users.noreply.github.com>
Co-authored-by: Dedmen Miller <dedmen@users.noreply.github.com>
Co-authored-by: Dedmen Miller <dedmen@users.noreply.github.com>
Co-authored-by: Dedmen Miller <dedmen@users.noreply.github.com>
Co-authored-by: Dedmen Miller <dedmen@users.noreply.github.com>
|
Seems Notepad++ must have done me dirty with the indentation... I've committed the suggestions (let me know if I didn't do it properly, I'm not well-versed with GitHub flows). Please review and merge if possible, once this module is up to date with my own version again I can maybe look at some of the other requests such as custom IDs, especially since I'll have some free time this week. Thanks! |
|
You could've merged them in one batch instead of several. You only merged half of my suggestions. If you click "Expand" above between my outdated ones, there are more CI errors
I don't know what that means Also
But, 38 has already been fixed by #40. |
| (28706, 16225, 16310.9, 29.262, 6.22119, 'First Aid Trainer (Grand Master)'), | ||
| (28742, 16225.3, 16313.9, 29.262, 6.28231, 'Fishing Trainer (Grand Master)'), | ||
| (33587, 16256.3, 16291.7, 13.174, 2.22426, 'Cooking Trainer (Grand Master)'), | ||
| (6491, 16319.937, 16242.404, 24.4747, 2.206830, 'Spirit Healer'), |
There was a problem hiding this comment.
| (6491, 16319.937, 16242.404, 24.4747, 2.206830, 'Spirit Healer'), | |
| (6491, 16319.937, 16242.404, 24.4747, 2.206830, 'Spirit Healer'), |
| (9858, 16240.5, 16291.5, 22.9318, 1.47417, 'Neutral Auctioneer'), | ||
| (184137, 16220.3, 16272, 12.9736, 4.45592, 'Mailbox (Object)'), | ||
| (187293, 16230.5, 16283.5, 13.9061, 3, 'Guild Vault (Object)'), | ||
| (191028, 16255.5, 16304.9, 20.9785, 2.97516, 'Barber Chair (Object)'), |
There was a problem hiding this comment.
| (191028, 16255.5, 16304.9, 20.9785, 2.97516, 'Barber Chair (Object)'), | |
| (191028, 16255.5, 16304.9, 20.9785, 2.97516, 'Barber Chair (Object)'), |
| (500005, 16231.3, 16254.2, 13.65647, 3.028813, 'Portal: Silvermoon (Object)'), | ||
| (500006, 16233.4, 16260.6, 13.84770, 3.028813, 'Portal: Thunder Bluff (Object)'), | ||
| (500007, 16232.8, 16257.1, 13.93456, 3.028813, 'Portal: Undercity (Object)'), | ||
| (500008, 16211.1, 16266.9, 13.7458, 5.6724, 'Portal: Shattrath (Object)'), |
There was a problem hiding this comment.
| (500008, 16211.1, 16266.9, 13.7458, 5.6724, 'Portal: Shattrath (Object)'), | |
| (500008, 16211.1, 16266.9, 13.7458, 5.6724, 'Portal: Shattrath (Object)'), |
| (500007, 16232.8, 16257.1, 13.93456, 3.028813, 'Portal: Undercity (Object)'), | ||
| (500008, 16211.1, 16266.9, 13.7458, 5.6724, 'Portal: Shattrath (Object)'), | ||
| (500009, 16213.9, 16270.5, 13.1378, 5.4996, 'Portal: Dalaran (Object)'), | ||
| (28690, 16226.8, 16269.4, 13.0858, 3.88255, 'Stable Master'), |
There was a problem hiding this comment.
| (28690, 16226.8, 16269.4, 13.0858, 3.88255, 'Stable Master'), | |
| (28690, 16226.8, 16269.4, 13.0858, 3.88255, 'Stable Master'), |
| (29636, 16233.2, 16315.9, 20.8454, 4.64365, 'Reagent Vendor'), | ||
| (29493, 16229.1, 16286.4, 13.176, 3.03831, 'Ammo & Repair Vendor'), |
There was a problem hiding this comment.
| (29636, 16233.2, 16315.9, 20.8454, 4.64365, 'Reagent Vendor'), | |
| (29493, 16229.1, 16286.4, 13.176, 3.03831, 'Ammo & Repair Vendor'), | |
| (29636, 16233.2, 16315.9, 20.8454, 4.64365, 'Reagent Vendor'), | |
| (29493, 16229.1, 16286.4, 13.176, 3.03831, 'Ammo & Repair Vendor'), |
| -- Updates existing creatures to the new creatures. | ||
| -- Replace <world> and <characters> with your database names! | ||
|
|
||
| -- The following only needs to be ran if you used this module before 5-1-2022. |
There was a problem hiding this comment.
I don't know why this was added? Seems like we shouldn't refer back to almost two years ago?
| cost = 0; | ||
|
|
||
| // Check if we need multiple trainers, a few require this otherwise | ||
| // you can't learn it soley in the guild house (Master is needed) |
There was a problem hiding this comment.
Can we really not just spawn the Master one and be done with it? Instead of spawning non-master AND master?
Have never tried that in game, don't know if possible
📝 WalkthroughWalkthroughGuild house spawn data, gossip actions, portal creation, innkeeper configuration, and AzerothCore object-data access were updated. Phase and teleport control branches were structurally reformatted while preserving their described behavior. ChangesGuild house updates
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Player
participant GuildHouseSpawner
participant OnGossipSelect
participant ObjectMgr
Player->>GuildHouseSpawner: Select trainer, object, or portal action
GuildHouseSpawner->>OnGossipSelect: Dispatch gossip action
OnGossipSelect->>ObjectMgr: Spawn NPC or gameobject data
ObjectMgr-->>Player: Created guild-house entity
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
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.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/mod_guildhouse.cpp (1)
244-252: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winBoth Stormwind and Orgrimmar starter portals are spawned for every guild, at identical coordinates.
SpawnStarterPortal(player, 500000)andSpawnStarterPortal(player, 500004)are called unconditionally here, but persql/world/guildhouse_spawns.sql, entries 500000 and 500004 share the exact same posX/posY/posZ/orientation. Every new guild house purchase — Alliance or Horde — ends up with two overlapping portal objects at the same spot, one of which is the wrong faction's city. A past reviewer's suggestion on this line implied picking the faction-appropriate entry rather than spawning both.🐛 Proposed fix
- // Spawn starter portals and the guild house butler automatically as part of purchase. - SpawnStarterPortal(player, 500000); // Stormwind - SpawnStarterPortal(player, 500004); // Orgrimmar + // Spawn the faction-appropriate starter portal and the guild house butler automatically as part of purchase. + if (player->GetTeamId() == TEAM_ALLIANCE) + { + SpawnStarterPortal(player, 500000); // Stormwind + } + else + { + SpawnStarterPortal(player, 500004); // Orgrimmar + }🤖 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/mod_guildhouse.cpp` around lines 244 - 252, Update the starter portal spawning in the guild-house purchase flow to select only the faction-appropriate portal entry instead of unconditionally calling SpawnStarterPortal for both 500000 and 500004. Use the purchasing player’s faction to choose the Stormwind or Orgrimmar entry, while preserving SpawnButlerNPC and the surrounding purchase behavior.
🤖 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 `@sql/world/guildhouse_spawns.sql`:
- Around line 136-138: Update the innkeeper-removal migration statement
targeting <world>.creature_template with entry 500032 to include a trailing
semicolon, matching the other SQL statements in the migration block.
In `@src/mod_guildhouse_butler.cpp`:
- Around line 156-158: Correct the user-facing gossip label in the
AddGossipItemFor call for entry 28701 from “Jewelcrafing Trainer” to
“Jewelcrafting Trainer”; leave the confirmation text and surrounding trainer
entries unchanged.
In `@src/mod_guildhouse.cpp`:
- Around line 78-84: Replace cr_data->id with cr_data->id1 in both
ObjectGuid::Create<HighGuid::Unit> calls: src/mod_guildhouse.cpp lines 78-84 and
282-288. Leave the surrounding creature lookup and deletion logic unchanged.
---
Outside diff comments:
In `@src/mod_guildhouse.cpp`:
- Around line 244-252: Update the starter portal spawning in the guild-house
purchase flow to select only the faction-appropriate portal entry instead of
unconditionally calling SpawnStarterPortal for both 500000 and 500004. Use the
purchasing player’s faction to choose the Stormwind or Orgrimmar entry, while
preserving SpawnButlerNPC and the surrounding purchase behavior.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5f26a3bf-8e8d-4d0b-ac43-83bab4aacb1a
📒 Files selected for processing (5)
conf/mod_guildhouse.conf.distsql/world/creatures_objects.sqlsql/world/guildhouse_spawns.sqlsrc/mod_guildhouse.cppsrc/mod_guildhouse_butler.cpp
💤 Files with no reviewable changes (1)
- conf/mod_guildhouse.conf.dist
| -- Remove innkeeper | ||
| DELETE FROM <world>.creature WHERE id1 = 500032 AND EXISTS (SELECT * FROM <characters>.guild_house where <characters>.guild_house.phase = <world>.creature.phaseMask); | ||
| DELETE FROM <world>.creature_template WHERE entry = 500032 |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Missing semicolon in the innkeeper-removal migration example.
Line 138 (DELETE FROM <world>.creature_template WHERE entry = 500032) has no trailing ;, unlike every other statement in this migration block. Anyone copy-pasting this documented migration snippet will hit a SQL syntax error.
🐛 Proposed fix
-DELETE FROM <world>.creature_template WHERE entry = 500032
+DELETE FROM <world>.creature_template WHERE entry = 500032;📝 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.
| -- Remove innkeeper | |
| DELETE FROM <world>.creature WHERE id1 = 500032 AND EXISTS (SELECT * FROM <characters>.guild_house where <characters>.guild_house.phase = <world>.creature.phaseMask); | |
| DELETE FROM <world>.creature_template WHERE entry = 500032 | |
| -- Remove innkeeper | |
| DELETE FROM <world>.creature WHERE id1 = 500032 AND EXISTS (SELECT * FROM <characters>.guild_house where <characters>.guild_house.phase = <world>.creature.phaseMask); | |
| DELETE FROM <world>.creature_template WHERE entry = 500032; |
🤖 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 `@sql/world/guildhouse_spawns.sql` around lines 136 - 138, Update the
innkeeper-removal migration statement targeting <world>.creature_template with
entry 500032 to include a trailing semicolon, matching the other SQL statements
in the migration block.
| AddGossipItemFor(player, GOSSIP_ICON_TRAINER, "Enchanting Trainer", GOSSIP_SENDER_MAIN, 28693, "Spawn Enchanting Trainer?", GuildHouseProf, false); | ||
| AddGossipItemFor(player, GOSSIP_ICON_TRAINER, "Jewelcrafing Trainer", GOSSIP_SENDER_MAIN, 28701, "Spawn Jewelcrafting Trainer?", GuildHouseProf, false); | ||
| AddGossipItemFor(player, GOSSIP_ICON_TRAINER, "Inscription Trainer", GOSSIP_SENDER_MAIN, 28702, "Spawn Inscription Trainer?", GuildHouseProf, false); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Typo: "Jewelcrafing" → "Jewelcrafting".
User-facing gossip text is missing a 't'.
✏️ Proposed fix
- AddGossipItemFor(player, GOSSIP_ICON_TRAINER, "Jewelcrafing Trainer", GOSSIP_SENDER_MAIN, 28701, "Spawn Jewelcrafting Trainer?", GuildHouseProf, false);
+ AddGossipItemFor(player, GOSSIP_ICON_TRAINER, "Jewelcrafting Trainer", GOSSIP_SENDER_MAIN, 28701, "Spawn Jewelcrafting Trainer?", GuildHouseProf, false);📝 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.
| AddGossipItemFor(player, GOSSIP_ICON_TRAINER, "Enchanting Trainer", GOSSIP_SENDER_MAIN, 28693, "Spawn Enchanting Trainer?", GuildHouseProf, false); | |
| AddGossipItemFor(player, GOSSIP_ICON_TRAINER, "Jewelcrafing Trainer", GOSSIP_SENDER_MAIN, 28701, "Spawn Jewelcrafting Trainer?", GuildHouseProf, false); | |
| AddGossipItemFor(player, GOSSIP_ICON_TRAINER, "Inscription Trainer", GOSSIP_SENDER_MAIN, 28702, "Spawn Inscription Trainer?", GuildHouseProf, false); | |
| AddGossipItemFor(player, GOSSIP_ICON_TRAINER, "Enchanting Trainer", GOSSIP_SENDER_MAIN, 28693, "Spawn Enchanting Trainer?", GuildHouseProf, false); | |
| AddGossipItemFor(player, GOSSIP_ICON_TRAINER, "Jewelcrafting Trainer", GOSSIP_SENDER_MAIN, 28701, "Spawn Jewelcrafting Trainer?", GuildHouseProf, false); | |
| AddGossipItemFor(player, GOSSIP_ICON_TRAINER, "Inscription Trainer", GOSSIP_SENDER_MAIN, 28702, "Spawn Inscription Trainer?", GuildHouseProf, false); |
🤖 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/mod_guildhouse_butler.cpp` around lines 156 - 158, Correct the
user-facing gossip label in the AddGossipItemFor call for entry 28701 from
“Jewelcrafing Trainer” to “Jewelcrafting Trainer”; leave the confirmation text
and surrounding trainer entries unchanged.
| uint32 lowguid = fields[0].Get<int32>(); | ||
| if (CreatureData const *cr_data = sObjectMgr->GetCreatureData(lowguid)) | ||
| { | ||
| if (Creature *creature = map->GetCreature(ObjectGuid::Create<HighGuid::Unit>(cr_data->id1, lowguid))) | ||
| if (Creature *creature = map->GetCreature(ObjectGuid::Create<HighGuid::Unit>(cr_data->id, lowguid))) | ||
| { | ||
| creature->CombatStop(); | ||
| creature->DeleteFromDB(); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== Files mentioning CreatureData ==\n'
rg -n --hidden --glob '!**/.git/**' '\bCreatureData\b' .
printf '\n== Likely definitions (CreatureData) ==\n'
rg -n --hidden --glob '!**/.git/**' 'struct\s+CreatureData|class\s+CreatureData|typedef.*CreatureData|using.*CreatureData' .
printf '\n== mod_guildhouse.cpp occurrences ==\n'
rg -n --hidden --glob '!**/.git/**' 'ObjectGuid::Create<HighGuid::Unit>\(cr_data->' src/mod_guildhouse.cpp
printf '\n== mod_guildhouse.cpp around reported lines ==\n'
sed -n '60,95p;270,300p' src/mod_guildhouse.cppRepository: azerothcore/mod-guildhouse
Length of output: 475
🌐 Web query:
AzerothCore CreatureData id1 id2 id3 struct definition
💡 Result:
In the AzerothCore source code, the CreatureData structure—defined in src/server/game/Entities/Creature/CreatureData.h—represents the spawn data for an individual creature instance, directly mapping to fields in the database's creature table [1][2][3]. The members id1, id2, and id3 are defined as uint32 types [1]. These fields store the entry IDs (referencing the creature_template table) for the creature [1]. While the database structure includes three ID fields to support potential variants or dynamic entry assignments, the core typically utilizes them for identifying the creature template entry [1][2][3]. The structure definition is as follows: struct CreatureData { CreatureData = default; uint32 id1{0}; // entry in creature_template uint32 id2{0}; // entry in creature_template uint32 id3{0}; // entry in creature_template uint16 mapid{0}; uint32 phaseMask{0}; uint32 displayid{0}; int8 equipmentId{0}; float posX{0.0f}; float posY{0.0f}; float posZ{0.0f}; float orientation{0.0f}; uint32 spawntimesecs{0}; float wander_distance{0.0f}; uint32 currentwaypoint{0}; uint32 curhealth{0}; uint32 curmana{0}; uint8 movementType{0}; uint8 spawnMask{0}; uint32 npcflag{0}; uint32 unit_flags{0}; // enum UnitFlags mask values uint32 dynamicflags{0}; uint32 ScriptId; bool dbData{true}; }; [1]
Citations:
- 1: https://github.com/trickerer/AzerothCore-wotlk-with-NPCBots/blob/06c2bddf/src/server/game/Entities/Creature/CreatureData.h
- 2: https://github.com/azerothcore/wiki/blob/master/docs/creature.md
- 3: https://www.azerothcore.org/wiki/creature
Use cr_data->id1 instead of cr_data->id
CreatureData exposes id1/id2/id3, not a plain id, so both ObjectGuid::Create<HighGuid::Unit>(cr_data->id, lowguid) calls will fail against AzerothCore:
src/mod_guildhouse.cpp#L78-L84src/mod_guildhouse.cpp#L282-L288
📍 Affects 1 file
src/mod_guildhouse.cpp#L78-L84(this comment)src/mod_guildhouse.cpp#L282-L288
🤖 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/mod_guildhouse.cpp` around lines 78 - 84, Replace cr_data->id with
cr_data->id1 in both ObjectGuid::Create<HighGuid::Unit> calls:
src/mod_guildhouse.cpp lines 78-84 and 282-288. Leave the surrounding creature
lookup and deletion logic unchanged.
2 participants
This removes the useless innkeeper and includes various other tweaks to the trainers. See commits for full details. This fixes #38.
Summary by CodeRabbit
New Features
Updates