Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Guild halls support #2467

Closed
wants to merge 11 commits into from
Closed

Guild halls support #2467

wants to merge 11 commits into from

Conversation

nekiro
Copy link
Member

@nekiro nekiro commented May 15, 2018

In this pull request I'm trying to add missing guild halls support.

What does this pr add?

  • now every guild can have it's own residence, which should be guild hall, but isn't required, it's up you to check that, keep in mind that you cannot set it from lua script, this should be done by AAC (because AAC creates new guild entry in database)
  • you can now call house:isGuildHall() to check whether it is guild hall or not
  • loading houses from houses.xml, will now parse missing "guildhall" attribute
  • houses now have one additional attribute called guildId, this attribute stores guild that owns this house, this should be used only with guild halls
  • made tfs load ownerid from database to determine who is the guild leader, this will fail if we have more than one leader in a guild, but currently the only other option is to check guildrank if its number 3, because 3 is leader.

Probably forgot about something as always, so let me know if anything is missing, btw. I didn't test this code.

New enums:
HOUSE_TYPE_NORMAL
HOUSE_TYPE_GUILDHALL

New lua methods:

  • guild:setHouseId() -- sets guild hall id
  • guild:getHouseId() -- gets guild hall id
  • house:getType() -- returns house type
  • house:setGuildId() -- sets guild id
  • house:getGuildId() -- gets guild id

@vankk
Copy link
Contributor

vankk commented May 15, 2018

I think that you missed the in-game part, should has a condition in House::setOwner that if the house is a guildHall and who is purchasing is a guild leader then should set was a guild house to the Guild. It may be good for thoses servers how uses !buyhouse command.

@malucooo
Copy link

malucooo commented Jun 3, 2018

need replace line 275 on iomapserialize.cpp
DBResult_ptr result = db.storeQuery("SELECT id, owner, paid, warnings FROM houses");
for this:
DBResult_ptr result = db.storeQuery("SELECT id, owner, paid, warnings, guildid FROM houses");

thanks for advance!

Copy link
Member

@DSpeichert DSpeichert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also read other comments, as in-game purchasing part is missing (it should not be deferred to AAC as the only method)

schema.sql Outdated
@@ -207,6 +208,7 @@ CREATE TABLE IF NOT EXISTS `houses` (
`highest_bidder` int(11) NOT NULL DEFAULT '0',
`size` int(11) NOT NULL DEFAULT '0',
`beds` int(11) NOT NULL DEFAULT '0',
`guildid` int(11) NOT NULL,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be guild_id

schema.sql Outdated
@@ -133,6 +133,7 @@ CREATE TABLE IF NOT EXISTS `guilds` (
`ownerid` int(11) NOT NULL,
`creationdata` int(11) NOT NULL,
`motd` varchar(255) NOT NULL DEFAULT '',
`residence` int(11) NOT NULL,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be house_id

src/guild.h Outdated
@@ -69,6 +69,12 @@ class Guild
void setMotd(const std::string& motd) {
this->motd = motd;
}
uint32_t getResidenceId() const {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of "residence" use "house", it's an actual entity in DB and really it's a house.

@nekiro
Copy link
Member Author

nekiro commented Jun 26, 2018

@vankk there was a reason why I didnt touch that. Currently tfs is only loading guilds from database (these guilds are created by AAC), we are not saving the guild from tfs in any scenario. So when we set the house id attribute to guild in tfs source, it won't get saved. I can write additional function that would be called after setting house id to guild, but I don't know if this is right approach.

schema.sql Outdated
@@ -133,6 +133,7 @@ CREATE TABLE IF NOT EXISTS `guilds` (
`ownerid` int(11) NOT NULL,
`creationdata` int(11) NOT NULL,
`motd` varchar(255) NOT NULL DEFAULT '',
`house_id` int(11) NOT NULL,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can't update schema.sql
It must be a migration.

schema.sql Outdated
@@ -207,6 +207,7 @@ CREATE TABLE IF NOT EXISTS `houses` (
`highest_bidder` int(11) NOT NULL DEFAULT '0',
`size` int(11) NOT NULL DEFAULT '0',
`beds` int(11) NOT NULL DEFAULT '0',
`guild_id` int(11) NOT NULL,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's also this one here that should be a migration instead.

@DSpeichert
Copy link
Member

@Znote can you take a look at this vs #2214 ?

@DSpeichert DSpeichert requested a review from WibbenZ February 17, 2019 18:03
@WibbenZ
Copy link
Member

WibbenZ commented Feb 17, 2019

LGTM

@DSpeichert DSpeichert requested a review from Znote February 18, 2019 15:02
@Znote
Copy link
Member

Znote commented Mar 12, 2019

Sorry, this got hidden in my backlog. Gonna take a proper look at it tonight/tomorrow. Sorry for the delay.

@Znote
Copy link
Member

Znote commented Mar 12, 2019

This vs #2214

  • This does not help an AAC understand what is and what isn't a guildhall. How do I differentiate between a normal house and a guildhall in the houses table (if they both are available)? This only adds an uneccesary reference?
  • Shouldn't it be possible to distinguish an available house from an available guildhall in-game?
  • This adds 4 bytes overhead to each houses rows + 4 bytes overhead to all guild rows. Mine only add 1 byte overhead to houses rows. (+8 to guilds with guild_bank functionality)
  • This only supports guild halls. Mine supports up to 128 different house group types, where 0 would be considered a house, 1 would be considered guildhalls, and 2+ for custom types. (etc 10+townid could be town hall rooms)? You can be more creative in Lua.
  • This does not support guild bank, mine does with an additional table and 8 byte bigint column in guilds row

@nekiro Have you looked at my request, do you think you could give the c++ code a go there?
if you could use house type instead, wrap regular house functions to use type 0, guildhalls to use type 1, and create a generic function which checks for configured type?

How do I get the guildid <-> houseid reference without the reference? I go through the player id. (owner)

Sample: 1:
All guildhalls, if guild_id is null then guildhall is available. (dont have a guild owner)

SELECT 
	`h`.`id`,
	`h`.`owner`,
	`g`.`id` AS `guild_id`
FROM `houses` AS `h` 
LEFT JOIN `guilds` AS `g` 
	ON `h`.`owner` = `g`.`ownerid`
	AND `h`.`type` = 1 
WHERE `h`.`type` = 1 

Sample 2:
All guilds, if house_id is null then guild does not have a guildhall

SELECT 
	`g`.`id`,
	`g`.`ownerid`,
	`h`.`id` AS `house_id`
FROM `guilds` AS `g`
LEFT JOIN `houses` AS `h`
	ON `g`.`ownerid` = `h`.`owner` 
	AND `h`.`type` = 1

If you need help with SQL code, I'm more than happy to provide more sample codes.

Imported houses where type is not specified should default to 0 for backwards compatibility. (So the SQL migration should be NOT NULL DEFAULT '0' to handle those inserts for houses.type).

Copy link
Member

@Znote Znote left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Talked to Nekiro in the staff discord channel;

Turns out I was wrong when I said it was impossible to distinguish houses and guildhalls from each other in-game, this is a problem in the database, as the sources load this information from XML.

There should be a way for an AAC with only database access to determine if a house is a guildhall, so house->setGuildHall(houseNode.attribute("guildhall") should be synced to db. Instead of some sort of bool is_guildhall, I recommend a tinyint type column as suggested in the code review.

This is to accomodate house groups and flexibility further down the line, and to give AAC makers a chance to distinguish different house categories by querying the database.

I suggest that regular houses to be set for type 0, while guildhalls be set as type 1.

With this type column, at least from the perspective of SQL querying, guild_id and house_id references is uneccesary. But I understand the desire to keep it in for simplicity in the sources.

data/migrations/23.lua Show resolved Hide resolved
@nekiro nekiro closed this Mar 15, 2019
@nekiro nekiro deleted the guilhalls-support branch March 15, 2019 18:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants