Stuck on a piece of code

[Criimson]

I need help with a problem from someone with more C++ exp.
I have some code that I just can't put the final cap on.

Here it is:

This code is in Bot::FillSpawnStruct

//CRIIMSON: Get BotID
		uint32 spawnedbotid = 0;
		spawnedbotid = this->GetBotID();
		Say("BotID = %i", spawnedbotid);

		inst = GetBotItem(SLOT_HANDS);
		if(inst) {
			item = inst->GetItem();
			if(item) {
				ns->spawn.equipment[MATERIAL_HANDS]	= item->Material;
				ns->spawn.colors[MATERIAL_HANDS].color = Bot::GetEquipmentColor(MATERIAL_HANDS, spawnedbotid);
				Say("Hands color = %i and spawnedbotid = %i", Bot::GetEquipmentColor(MATERIAL_HANDS, spawnedbotid), spawnedbotid);
			}
		}

There are more calls, but they all are similar.
And this gets called

uint32 Bot::GetEquipmentColor(int8 material_slot, uint32 botid) const
{
	//CRIIMSON: Bot tints
	int32 slotid = 0;
	uint32 returncolor = 0;
	
	//Translate code slot # to DB slot #
	while(slotid == 0){
		if (material_slot == 0){
			slotid = 2;
		}
		else if (material_slot == 1){
			slotid = 17;
		}
		else if (material_slot == 2){
			slotid = 7;
		}
		else if (material_slot == 3){
			slotid = 9;
		}
		else if (material_slot == 4){
			slotid = 12;
		}
		else if (material_slot == 5){
			slotid = 18;
		}
		else if (material_slot == 6){
			slotid = 19;
		}

	//CRIIMSON: next two won't be called usually - put in for servers that want to allow dying of weaps/shields
		else if (material_slot == 7){
			slotid = 13;
		}
		else if (material_slot == 8){
			slotid = 14;
		}
	}

	//CRIIMSON: read from db
	char* Query = 0;
	MYSQL_RES* DatasetResult;
	MYSQL_ROW DataRow;
	
	if(database.RunQuery(Query, MakeAnyLenString(&Query, "SELECT color FROM botinventory WHERE BotID = %i AND SlotID = %i", botid, slotid), 0, &DatasetResult)) {
		if(mysql_num_rows(DatasetResult) == 1) {
			DataRow = mysql_fetch_row(DatasetResult);
			if(DataRow)
				returncolor = atoi(DataRow[0]);
		}
		mysql_free_result(DatasetResult);
		safe_delete_array(Query);
	}
	return returncolor;
}

Now the problem is a catch 22.

If I remove the const on Bot::GetEquipmentColor (as it is above) the color value is transfered but it doesnt show in game

If I leave it a const I cant put a pointer in the function to check botid (because the ID can change) so I tried to add the second variable sent to it. This sends and retieves the correct info from db but again it doesnt show in game.

Which is weird because the slotid changes fine even when defined as a const

I know the function called like:
uint32 Bot::GetEquipmentColor(int8 material_slot) const
will change all of the colors of armor (and shows in game) if I set the return as a number like 1644825 (which is black). However, this wont allow a botid variable which is neccessary to get the correct bots color scheme.

The only way I get it to work is not sending botid or having a pointer in the const to botid.

going friggin crazy over this.
So close I can taste it.

Criimson

[lerxst2112]

Removing the const from the function just makes it not match the existing virtual function so it isn't called from base class pointers.

That means Mob::SendWearChange can't call your new function which is where it seems the actual work of changing the colors happens.

You said the ID you have at the time FillSpawnStruct is called isn't the same ID that the bot has once it is spawned? Have you considered using GetBotIDByBotName to look it up inside your new Bot::GetEquipmentColor function?

I'm also a little confused why the part where you are setting the slotid is inside a while loop. In the best case where a valid material slot is passed the while loop has no effect. If for whatever reason an invalid material slot is passed it's an endless loop. You should probably replace all of that with Inventory::CalcSlotFromMaterial() anyway.

[Criimson]

No the ID passed is always correct
I just can't call it from the const using a pointer such as
botid = this->GetBotID(); //this causes an error at compile
I can send botid like the code above but it doesn't show in game on the bots. Meaning the Say("Checks") all show that every variable has exactly what it is supposed to have but it isnt showing.
To test if my code worked in some degree what I did was:

uint32 Bot::GetEquipmentColor(int8 material_slot) const
{
	//CRIIMSON: Bot tints
	int32 slotid = 0;
	uint32 returncolor = 0;
              botid = 0;
	
	//Translate code slot # to DB slot #
	while(slotid == 0){
		if (material_slot == 0){
			slotid = 2;
		}
		else if (material_slot == 1){
			slotid = 17;
		}
		else if (material_slot == 2){
			slotid = 7;
		}
		else if (material_slot == 3){
			slotid = 9;
		}
		else if (material_slot == 4){
			slotid = 12;
		}
		else if (material_slot == 5){
			slotid = 18;
		}
		else if (material_slot == 6){
			slotid = 19;
		}

	//CRIIMSON: next two won't be called usually - put in for servers that want to allow dying of weaps/shields
		else if (material_slot == 7){
			slotid = 13;
		}
		else if (material_slot == 8){
			slotid = 14;
		}
	}

	//CRIIMSON: read from db
	char* Query = 0;
	MYSQL_RES* DatasetResult;
	MYSQL_ROW DataRow;
	
	if(database.RunQuery(Query, MakeAnyLenString(&Query, "SELECT color FROM botinventory WHERE BotID = %i AND SlotID = %i", botid, slotid), 0, &DatasetResult)) {
		if(mysql_num_rows(DatasetResult) == 1) {
			DataRow = mysql_fetch_row(DatasetResult);
			if(DataRow)
				returncolor = atoi(DataRow[0]);
		}
		mysql_free_result(DatasetResult);
		safe_delete_array(Query);
	}
	//return returncolor;
             return 1644825;
}

Which doesn't read the DB and always sends back black, but every bot is wearing all black when spawned or zoned. Which leads me to believe that the code I have is functioning and if I could figure out how to get botid into the function all would be well and there would be much happiness in my heart

Criimson

EDIT: and I renamed the function GetBotEquipmentColor to keep people from getting it confused with the ones in MoB:: or Client::

EDIT EDIT:
The while statement was for convenience. It will always show == 0 on call and it is only called by the items that show on bots so there is no chance of a loop happening as my code stands now.

[lerxst2112]

Make GetBotID const.

uint32 GetBotID() const { return _botID; }

You can always call a const function, but you can't call a non-const function from a const function.

Actually, looking at the code all of these should be const. If you fix them in the header you'll have to fix the two non-inline functions in the cpp file as well.

	// "GET" Class Methods
	uint32 GetBotID() const { return _botID; }
	uint32 GetBotOwnerCharacterID() const { return _botOwnerCharacterID; }
	uint32 GetBotSpellID() const { return npc_spells_id; }
	Mob* GetBotOwner() const { return this->_botOwner; }
	uint32 GetBotArcheryRange() const;
	ItemInst* GetBotItem(uint32 slotID) const;
	virtual bool GetSpawnStatus() const { return _spawnStatus; }
	int8 GetPetChooserID() const { return _petChooserID; }
	bool IsPetChooser() const { return _petChooser; }
	bool IsBotArcher() const { return _botArcher; }
	bool IsBotCharmer() const { return _botCharmer; }
	virtual bool IsBot() const { return true; }
	bool GetRangerAutoWeaponSelect() const { return _rangerAutoWeaponSelect; }
	BotRoleType GetBotRole() const { return _botRole; }
	bool IsBotCaster() const { return (GetClass() == CLERIC || GetClass() == DRUID || GetClass() == SHAMAN || GetClass() == NECROMANCER || GetClass() == WIZARD || GetClass() == MAGICIAN || GetClass() == ENCHANTER); }
	bool IsBotINTCaster() const { return (GetClass() == NECROMANCER || GetClass() == WIZARD || GetClass() == MAGICIAN || GetClass() == ENCHANTER); }
	bool IsBotWISCaster() const { return (GetClass() == CLERIC || GetClass() == DRUID || GetClass() == SHAMAN); }
	inline virtual sint16	GetAC()	const { return AC; }

[lerxst2112]

Originally Posted by Criimson

EDIT: and I renamed the function GetBotEquipmentColor to keep people from getting it confused with the ones in MoB:: or Client::

EDIT EDIT:
The while statement was for convenience. It will always show == 0 on call and it is only called by the items that show on bots so there is no chance of a loop happening as my code stands now.

I am pretty sure you want to override the existing virtual function GetEquipmentColor since it is called from several places that may need to get the proper color.

I still don't get the convenience part of the while loop. It doesn't do anything useful except introduce the possibility for an infinite loop. Even though the existing code may not cause a problem for you today you should always be pragmatic and check your inputs to protect against error.

Anyway, if you replace it with the existing function Inventory::CalcSlotFromMaterial() that does the same thing then your code is cleaner, doesn't rely on magic numbers, and doesn't violate the DRY principle.

[Criimson]

Originally Posted by lerxst2112

I am pretty sure you want to override the existing virtual function GetEquipmentColor since it is called from several places that may need to get the proper color.

I may be wrong, but when creating a function and defining it doesn't it gain autonomy from functions that have a different name, even if they are similar?
Edit: This is all a learning experience for me. My C++ is still at a novice level. I am learning by reading the code and in this project there is a lot of it

I still don't get the convenience part of the while loop. It doesn't do anything useful except introduce the possibility for an infinite loop. Even though the existing code may not cause a problem for you today you should always be pragmatic and check your inputs to protect against error.

You are totally correct. I am removing the while statement.

Anyway, if you replace it with the existing function Inventory::CalcSlotFromMaterial() that does the same thing then your code is cleaner, doesn't rely on magic numbers, and doesn't violate the DRY principle.

Not sure what you mean in two parts. I'll google the DRY principle.
What magic numbers?

Criimson

EDIT: Nice function - will totally use that, thanks

[lerxst2112]

Yes, if you name the function differently or change its signature then it isn't connected to the functions in base classes, but then you also aren't going to have it called from Mob::FillSpawnStruct or Mob::SendWearChange which might be important.

The call from Mob::FillSpawnStruct looks like it probably isn't important because the data is overwritten right after that by the Bot function, but Mob::SendWearChange looks more important. If it can't call the bot function the colors may not show up, or they may disappear when the bot changes equipment.


Magic numbers are whenever you use a hard coded number instead of a name for something in an enum or #define.

//Magic numbers
if (material_slot == 0)
    slotid = 2;

//Safe if someone renumbered the material Ids or slot numbers
if (material_slot == MATERIAL_HEAD)
    slotid = SLOT_HEAD;

It just means if someone decided to renumber the material IDs or slot numbers for some reason they wouldn't have to examine every 0 or 2 in the code and decide if it needed to change.

[Criimson]

Well I took your advice.

Had to change the name back to GetEquipmentColor

But redid the material to the function you suggested and then created a const GetBotIDForTint variable.
Called it from the function and now the bots have dyed armor.

Perfect.

Kind of tired and I haven't ran an LDoN or fought much today so will complete the #bot armorcolor function tomorrow.

Have it at a basic working right now.

Thank you for the help. It was requested for bots and I learned a ton working on it.

Criimson

[Criimson]

It just means if someone decided to renumber the material IDs or slot numbers for some reason they wouldn't have to examine every 0 or 2 in the code and decide if it needed to change.

That makes sense. To be honest it took me forever to wrap my head around object oriented thinking. I first coded on my C64 in BASIC and was so used to linear thinking. It wasn't until I started reading others C++ code that it began to sink in.