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

Monster icons #4365

Closed
wants to merge 2 commits into from
Closed

Monster icons #4365

wants to merge 2 commits into from

Conversation

ArturKnopik
Copy link
Contributor

@ArturKnopik ArturKnopik commented Apr 2, 2023

Co-Authored-By: @Zbizu

creature:hasIcon(iconId)
creature:getIconValue(iconId)
creature:setIconValue(iconId, value)
creature:removeIcon(iconId)

monster:hasMonsterIcon(iconId)
monster:getMonsterIconValue(iconId)
monster:setMonsterIconValue(iconId, value)
monster:removeMonsterIcon(iconId)

image

Pull Request Prelude

  • I have followed [proper The Forgotten Server code styling][code].
  • I have read and understood the [contribution guidelines][cont] before making this PR.
  • I am aware that this PR may be closed if the above-mentioned criteria are not fulfilled.

@@ -8615,6 +8655,71 @@ int LuaScriptInterface::luaCreatureGetZone(lua_State* L)
return 1;
}

int LuaScriptInterface::luaCreatureHasIcon(lua_State* L)
Copy link
Contributor

@MillhioreBT MillhioreBT Apr 2, 2023

Choose a reason for hiding this comment

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

I think you should follow the format of the other methods:

if (!userdata) {
lua_pushnil();
return 1;
}

-- Code

SpectatorVec spectators;
g_game.map.getSpectators(spectators, this->getPosition(), true, true);
for (Creature* spectator : spectators) {
if (auto player = spectator->getPlayer()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that here should also be verified in this way:

assert(dynamic_cast<Player*>(spectator) != nullptr);
static_cast<Player*>(spectator)->sendUpdateCreatureIcons(this);

Copy link
Contributor

Choose a reason for hiding this comment

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

Spectator existing and being player is already verified in getSpectators (onlyPlayers param). Would be nice if PR author could at least mention where he got that code from.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Idk from where i took this code, it's possible it is your code :)
i have it for a while... :D

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image

@EPuncker
Copy link
Contributor

EPuncker commented Apr 2, 2023

nice!

@EPuncker EPuncker added the feature New feature or functionality label Apr 2, 2023
@MillhioreBT
Copy link
Contributor

MillhioreBT commented Apr 6, 2023

I would like the names of the methods to be a little better...
Example:
Creature.getIcons
Monster.getSpecialIcons

Since the redundancy of names is something strange.
Creature.getCreatureIcons
Monster.getMonsterIcons

Unfortunately, they cannot be used generically as it would make it more difficult to manage. However, I think this should have a more appropriate name. maybe?

It's worth mentioning that in the following examples I've used C++20 contains, but you can change it to find() != end()

Other Things...

Wouldn't it be better to have two methods to handle or read the maps? this way they would be handled directly without creating methods that are never actually used on the sources side, they will only be used in lua

creature.h
using CreatureIconHashMap = std::unordered_map<CreatureIcon_t, uint16_t>;

	// creature icons
	CreatureIconHashMap& getIcons() { return creatureIcons; }
	const CreatureIconHashMap& getIcons() const { return creatureIcons; }
	void updateIcons() const;

protected:
	CreatureIconHashMap creatureIcons;
monster.h
using MonsterIconHashMap = std::unordered_map<MonsterIcon_t, uint16_t>;

	// monster icons
	MonsterIconHashMap& getSpecialIcons() { return monsterIcons; }
	const MonsterIconHashMap& getSpecialIcons() const { return monsterIcons; }

private:
	MonsterIconHashMap monsterIcons;
luascript - creature
int LuaScriptInterface::luaCreatureHasIcon(lua_State* L)
{
	// creature:hasIcon(iconId)
	const Creature* creature = getUserdata<const Creature>(L, 1);
	if (creature) {
		pushBoolean(L, creature->getIcons().contains(getNumber<CreatureIcon_t>(L, 2)));
	} else {
		lua_pushnil(L);
	}
	return 1;
}

int LuaScriptInterface::luaCreatureSetIcon(lua_State* L)
{
	// creature:setIcon(iconId, value)
	Creature* creature = getUserdata<Creature>(L, 1);
	if (!creature) {
		lua_pushnil(L);
		return 1;
	}

	auto iconId = getNumber<CreatureIcon_t>(L, 2);
	if (iconId >= CREATURE_ICON_LAST) {
		reportErrorFunc(L, "Invalid Creature Icon Id");
		pushBoolean(L, false);
		return 1;
	}

	creature->getIcons()[iconId] = getNumber<uint16_t>(L, 3);
	creature->updateIcons();
	pushBoolean(L, true);
	return 1;
}

int LuaScriptInterface::luaCreatureGetIcon(lua_State* L)
{
	// creature:getIcon(iconId)
	Creature* creature = getUserdata<Creature>(L, 1);
	if (!creature) {
		lua_pushnil(L);
		return 1;
	}

	auto iconId = getNumber<CreatureIcon_t>(L, 2);
	const auto& icons = creature->getIcons();
	if (icons.contains(iconId)) {
		lua_pushinteger(L, icons.at(iconId));
	} else {
		lua_pushinteger(L, 0);
	}
	return 1;
}

int LuaScriptInterface::luaCreatureRemoveIcon(lua_State* L)
{
	// creature:removeIcon(iconId)
	Creature* creature = getUserdata<Creature>(L, 1);
	if (!creature) {
		lua_pushnil(L);
		return 1;
	}

	auto iconId = getNumber<CreatureIcon_t>(L, 2);
	auto& icons = creature->getIcons();
	if (icons.contains(iconId)) {
		icons.erase(iconId);
		creature->updateIcons();
		pushBoolean(L, true);
	} else {
		pushBoolean(L, false);
	}
	return 1;
}
luascript - monster
int LuaScriptInterface::luaMonsterHasIcon(lua_State* L)
{
	// monster:hasSpecialIcon(iconId)
	const Monster* monster = getUserdata<const Monster>(L, 1);
	if (monster) {
		pushBoolean(L, monster->getSpecialIcons().contains(getNumber<MonsterIcon_t>(L, 2)));
	} else {
		lua_pushnil(L);
	}
	return 1;
}

int LuaScriptInterface::luaMonsterSetIcon(lua_State* L)
{
	// monster:setSpecialIcon(iconId, value)
	Monster* monster = getUserdata<Monster>(L, 1);
	if (!monster) {
		lua_pushnil(L);
		return 1;
	}

	auto iconId = getNumber<MonsterIcon_t>(L, 2);
	if (iconId >= MONSTER_ICON_LAST) {
		reportErrorFunc(L, "Invalid Monster Icon Id");
		pushBoolean(L, false);
		return 1;
	}

	monster->getSpecialIcons()[iconId] = getNumber<uint16_t>(L, 3);
	monster->updateIcons();
	pushBoolean(L, true);
	return 1;
}

int LuaScriptInterface::luaMonsterGetIcon(lua_State* L)
{
	// monster:getSpecialIcon(iconId)
	Monster* monster = getUserdata<Monster>(L, 1);
	if (!monster) {
		lua_pushnil(L);
		return 1;
	}

	auto iconId = getNumber<MonsterIcon_t>(L, 2);
	const auto& icons = monster->getSpecialIcons();
	if (icons.contains(iconId)) {
		lua_pushinteger(L, icons.at(iconId));
	} else {
		lua_pushinteger(L, 0);
	}
	return 1;
}

int LuaScriptInterface::luaMonsterRemoveIcon(lua_State* L)
{
	// monster:removeSpecialIcon(iconId)
	Monster* monster = getUserdata<Monster>(L, 1);
	if (!monster) {
		lua_pushnil(L);
		return 1;
	}

	auto iconId = getNumber<MonsterIcon_t>(L, 2);
	auto& icons = monster->getSpecialIcons();
	if (icons.contains(iconId)) {
		icons.erase(iconId);
		monster->updateIcons();
		pushBoolean(L, true);
	} else {
		pushBoolean(L, false);
	}
	return 1;
}
protocolgame.cpp
void ProtocolGame::AddCreatureIcons(NetworkMessage& msg, const Creature* creature)
{
	const auto& creatureIcons = creature->getIcons();
	if (const Monster* monster = creature->getMonster()) {
		const auto& monsterIcons = monster->getSpecialIcons();
		msg.addByte(creatureIcons.size() + monsterIcons.size());
		for (const auto& [iconId, level] : monsterIcons) {
			msg.addByte(iconId);
			msg.addByte(1);
			msg.add<uint16_t>(level);
		}
	} else {
		msg.addByte(creatureIcons.size());
	}

	for (const auto& [iconId, level] : creatureIcons) {
		msg.addByte(iconId);
		msg.addByte(0);
		msg.add<uint16_t>(level);
	}
}

@MillhioreBT MillhioreBT mentioned this pull request Jun 25, 2023
3 tasks
@ArturKnopik
Copy link
Contributor Author

covered by #4493

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants