Default all localized behavior to European logic #22

Merged
Exynox merged 20 commits from Tr0n/server:localization/default-gf-eu into nightly 2024-04-09 21:47:23 +03:00
Contributor

Removes all localized behavior in favor of what was enabled for EU.
Only exception to this is the 6th and 7th bonus, which was disabled for EU.
Includes removal of Chinese 3 and 5 hour limits.

g_iUseLocale and LC_Is* calls were removed entirely.

Removes all localized behavior in favor of what was enabled for EU. Only exception to this is the 6th and 7th bonus, which was disabled for EU. Includes removal of Chinese 3 and 5 hour limits. `g_iUseLocale` and `LC_Is*` calls were removed entirely.
Tr0n added 2 commits 2024-04-07 19:00:09 +03:00
Tr0n added 1 commit 2024-04-07 19:10:28 +03:00
Owner

Thanks, I'll check it out when I have time!

Thanks, I'll check it out when I have time!
Tr0n added 1 commit 2024-04-07 23:11:26 +03:00
Exynox reviewed 2024-04-08 21:28:04 +03:00
@ -1099,36 +1099,6 @@ bool CArena::RegisterObserverPtr(LPCHARACTER pChar)
bool CArenaManager::IsLimitedItem( int lMapIndex, DWORD dwVnum )
Owner

If this function always returns false, shouldn't we better remove it altogether?

If this function always returns false, shouldn't we better remove it altogether?
Tr0n marked this conversation as resolved
Exynox reviewed 2024-04-08 21:31:43 +03:00
@ -870,3 +864,2 @@
addPacket.dwLevel = 0;
}
//TODO: what is this doing here?
Owner

How weird! I suppose it can be removed, including that goto.

How weird! I suppose it can be removed, including that goto.
Tr0n marked this conversation as resolved
Exynox reviewed 2024-04-08 21:34:13 +03:00
@ -124,4 +115,0 @@
int iVal;
if (!g_iUseLocale)
iVal = std::min(GetPoint(POINT_SP_RECOVERY), GetMaxSP() * 7 / 100);
Owner

How weird that these two branches of the "if" were exactly the same!

How weird that these two branches of the "if" were exactly the same!
Exynox marked this conversation as resolved
Exynox reviewed 2024-04-08 21:37:19 +03:00
@ -2102,3 +2051,1 @@
percent = 8;
else
percent = 9;
percent = 9;
Owner

Isn't percent always set to 9 here? I suppose we could just remove the if and leave percent = 9;

Isn't percent always set to 9 here? I suppose we could just remove the if and leave `percent = 9;`
Tr0n marked this conversation as resolved
Exynox reviewed 2024-04-08 21:39:00 +03:00
@ -2182,4 +2127,0 @@
// -----------------------
if (LC_IsGermany() && pAttacker && pAttacker->IsPC())
{
int iDmgPct = CHARACTER_MANAGER::instance().GetUserDamageRate(pAttacker);
Owner

So I suppose some players in Germany could've had extra damage as a premium feature? How broken and P2W!

So I suppose some players in Germany could've had extra damage as a premium feature? How broken and P2W!
Exynox marked this conversation as resolved
Exynox reviewed 2024-04-08 21:42:39 +03:00
@ -146,24 +146,6 @@ bool IS_SUMMONABLE_ZONE(int map_index)
return true;
}
bool IS_BOTARYABLE_ZONE(int nMapIndex)
Owner

I wonder what this was used for...

I wonder what this was used for...
Owner

I see, in the YMIR locale you could only open private shops in map1/2. This reminds me of the current behaviour on present day Gameforge servers, where you can only open shops in the Bazaar.

I see, in the YMIR locale you could only open private shops in map1/2. This reminds me of the current behaviour on present day Gameforge servers, where you can only open shops in the Bazaar.
Author
Contributor

do you want me to revert this?

do you want me to revert this?
Owner

Honestly, it's fine this way. If somebody ever needs this feature, CHARACTER::__OpenPrivateShop() would be a much better to implement it anyway.

Honestly, it's fine this way. If somebody ever needs this feature, `CHARACTER::__OpenPrivateShop()` would be a much better to implement it anyway.
Exynox marked this conversation as resolved
Exynox reviewed 2024-04-08 21:46:13 +03:00
@ -2611,3 +2488,1 @@
bi = b2;
else
bi = b1;
LuckyBagInfo * bi = b1;
Owner

b1 could be renamed to bi, I suppose, therefore removing this pointer?

`b1` could be renamed to `bi`, I suppose, therefore removing this pointer?
Author
Contributor

sure. will do.

sure. will do.
Tr0n marked this conversation as resolved
Exynox reviewed 2024-04-08 21:47:38 +03:00
@ -3803,10 +3664,6 @@ bool CHARACTER::UseItemEx(LPITEM item, TItemPos DestCell)
case 71051 : // 진재가
{
// 유럽, 싱가폴, 베트남 진재가 사용금지
Owner

I see, these were the 6/7 bonuses being banned here.

I see, these were the 6/7 bonuses being banned here.
Exynox marked this conversation as resolved
Exynox reviewed 2024-04-08 21:51:11 +03:00
@ -402,13 +380,7 @@ bool CHARACTER::LearnGrandMasterSkill(DWORD dwSkillVnum)
static bool FN_should_check_exp(LPCHARACTER ch)
Owner

Function always returns true, therefore it can be removed.

Function always returns true, therefore it can be removed.
Author
Contributor

I will remove it entirely. Thought it would be good to still have the provision to implement own logic.
But you're right coding for what you "might do" in the future is not good.

I will remove it entirely. Thought it would be good to still have the provision to implement own logic. But you're right coding for what you "might do" in the future is not good.
Tr0n marked this conversation as resolved
Exynox reviewed 2024-04-08 21:55:26 +03:00
@ -2269,3 +2234,1 @@
ch->ChatPacket(CHAT_TYPE_COMMAND, "mall http://metin2.co.kr/04_mall/mall/login.htm");
return;
}
char country_code[3];
Owner

I'll refactor this sometime in the future, in order to provide a secure authentication mechanism to the CMS I'm working on.

I'll refactor this sometime in the future, in order to provide a secure authentication mechanism to the CMS I'm working on.
Exynox marked this conversation as resolved
Exynox reviewed 2024-04-08 21:59:18 +03:00
@ -394,4 +390,0 @@
return 0;
}
if (g_iUseLocale) // 중국에서는 금덩어리, 금열쇠, 은열쇠 나오지 않게 함
Owner

Shouldn't this have been kept?? g_iUseLocale is true for Europe.

Shouldn't this have been kept?? `g_iUseLocale` is true for Europe.
Author
Contributor

absolutely, my bad.

absolutely, my bad.
Tr0n marked this conversation as resolved
Exynox reviewed 2024-04-08 22:02:51 +03:00
@ -538,12 +538,9 @@ void CGuildManager::DeclareWar(DWORD guild_id1, DWORD guild_id2, BYTE bType)
if (g1->DeclareWar(guild_id2, bType, GUILD_WAR_SEND_DECLARE) &&
g2->DeclareWar(guild_id1, bType, GUILD_WAR_RECV_DECLARE))
{
if (false == LC_IsGermany())
Owner

Why did they disable this in Germany, lol!?

Why did they disable this in Germany, lol!?
Exynox marked this conversation as resolved
Exynox reviewed 2024-04-08 22:07:48 +03:00
@ -442,3 +434,2 @@
}
const TAccountTable & c_rAccountTable = d->GetAccountTable();
d->Packet(&packFailure, sizeof(packFailure));
Owner

I think there's an issue here, it looks like d->Packet(&packFailure, sizeof(packFailure)); should have remained, including the return statement and everything else. Only after the closing bracket would const TAccountTable & c_rAccountTable = d->GetAccountTable(); follow.

I think there's an issue here, it looks like `d->Packet(&packFailure, sizeof(packFailure));` should have remained, including the return statement and everything else. Only after the closing bracket would `const TAccountTable & c_rAccountTable = d->GetAccountTable();` follow.
Author
Contributor

good catch, thanks. will revert.

good catch, thanks. will revert.
Tr0n marked this conversation as resolved
Exynox reviewed 2024-04-08 22:10:01 +03:00
@ -653,4 +617,3 @@
}
else
{
SPDLOG_WARN("VERSION : NO CHECK");
Owner

I see you've removed these warnings as well. I guess they aren't of much use after all.

I see you've removed these warnings as well. I guess they aren't of much use after all.
Author
Contributor

that was by accident. will restore.

that was by accident. will restore.
Tr0n marked this conversation as resolved
Exynox reviewed 2024-04-08 22:10:52 +03:00
@ -723,3 +712,3 @@
if (pinfo->type == CHAT_TYPE_SHOUT)
{
const int SHOUT_LIMIT_LEVEL = g_iUseLocale ? 15 : 3;
const int SHOUT_LIMIT_LEVEL = 15;
Owner

It would also be nice to have this configurable

It would also be nice to have this configurable
Exynox marked this conversation as resolved
Exynox reviewed 2024-04-08 22:11:35 +03:00
@ -2444,3 +2420,1 @@
{
GuildCreateFee = 200000;
}
GuildCreateFee = 200000;
Owner

This should be configurable as well!

This should be configurable as well!
Exynox marked this conversation as resolved
Exynox reviewed 2024-04-08 22:18:49 +03:00
@ -1073,4 +1035,0 @@
}
//
// 승룡곡 1층, 2층에서만 7,8 스킬입문서 드롭
Owner

Interesting, I wonder if these "7/8" skills were these beta ones: https://metin2.dev/topic/20822-ymir-beta-skills/

Interesting, I wonder if these "7/8" skills were these beta ones: https://metin2.dev/topic/20822-ymir-beta-skills/
Exynox marked this conversation as resolved
Exynox reviewed 2024-04-08 22:22:19 +03:00
@ -6,8 +6,6 @@ typedef std::map< std::string, std::string > LocaleStringMapType;
LocaleStringMapType localeString;
int g_iUseLocale = 0;
Owner

I would have kept this variable and used it for English, so as in locale_find() to skip checking the locale string map.

I would have kept this variable and used it for English, so as in locale_find() to skip checking the locale string map.
Author
Contributor

While I think the point for having a locale file for English to English being pointless is fair, I also believe it to be valuable to have all locales being treated the same.
That way you would be "forced" (not really if you don't care about errors in the logs) to keep your locale_string up to date with all messages you used in the code.
Therefore you have a much easier time down the line, when you do decide to localize all messages, as you don't have to look through the code for all your custom messages: you should have added them to the locale_string from the beginning.

While I think the point for having a locale file for English to English being pointless is fair, I also believe it to be valuable to have all locales being treated the same. That way you would be "forced" (not really if you don't care about errors in the logs) to keep your locale_string up to date with all messages you used in the code. Therefore you have a much easier time down the line, when you do decide to localize all messages, as you don't have to look through the code for all your custom messages: you should have added them to the locale_string from the beginning.
Owner

Your argument is fair and I've thought of that as well. The thing is that once the multilanguage system will be implemented, this behaviour will have to be reverted, as the English strings will be converted to [LS;id] strings as Mali showcases in his topic on metin2.dev: https://metin2.dev/topic/29790-official-client-locale-stringreversed/

Therefore, the errors will show up in the log if a translation string ID is not configured in the server.

Not to mention that the 2024 agile software development good practices™ (patent pending) thing to do would be to have a CI job to check for this kind of issues. But as the saying goes here, we'll eat a bread or two by the time that happens.

Your argument is fair and I've thought of that as well. The thing is that once the multilanguage system will be implemented, this behaviour will have to be reverted, as the English strings will be converted to `[LS;id]` strings as Mali showcases in his topic on metin2.dev: https://metin2.dev/topic/29790-official-client-locale-stringreversed/ Therefore, the errors will show up in the log if a translation string ID is not configured in the server. Not to mention that the 2024 agile software development good practices™ (patent pending) thing to do would be to have a CI job to check for this kind of issues. But as the saying goes here, we'll eat a bread or two by the time that happens.
Author
Contributor

All good, since you're way more involved in the m2 dev scene I will definitely follow your judgement here 👍

All good, since you're way more involved in the m2 dev scene I will definitely follow your judgement here 👍
Tr0n marked this conversation as resolved
Exynox reviewed 2024-04-08 22:23:28 +03:00
@ -34,3 +27,3 @@
SPDLOG_ERROR("LOCALE_ERROR: \"{}\";", string);
return s_line;
return string;
Owner

I don't get what happened here and what warrants the change from s_line to string?

I don't get what happened here and what warrants the change from s_line to string?
Author
Contributor

my bad. Was testing something locally and forgot to revert. Will restore original functionality.

my bad. Was testing something locally and forgot to revert. Will restore original functionality.
Tr0n marked this conversation as resolved
Exynox reviewed 2024-04-08 22:25:29 +03:00
@ -479,4 +419,0 @@
g_setQuestObjectDir.clear();
g_setQuestObjectDir.insert("locale/germany/quest/object");
g_stLocaleFilename = "locale/germany/locale_string.txt";
Owner

I think we should better keep these init functions, so as for example locale_string_de.txt is loaded when German is specified, locale_string_fr.txt for French, and so on and so forth...

I think we should better keep these init functions, so as for example locale_string_de.txt is loaded when German is specified, locale_string_fr.txt for French, and so on and so forth...
Author
Contributor

i restored them and made the English locale the default. Let me know if that implementation fits what you had in mind.

i restored them and made the English locale the default. Let me know if that implementation fits what you had in mind.
Owner

Yeah, this works for now as a stop-gap solution until the multilanguage system is implemented in the client side.

Yeah, this works for now as a stop-gap solution until the multilanguage system is implemented in the client side.
Tr0n marked this conversation as resolved
Exynox reviewed 2024-04-08 22:46:30 +03:00
@ -683,4 +680,0 @@
if (teen_addr[0] && teen_port)
g_TeenDesc = DESC_MANAGER::instance().CreateConnectionDesc(ev_base, dns_base, teen_addr, teen_port, PHASE_TEEN, true);
extern unsigned int g_uiSpamBlockDuration;
Owner

I think the spam-related stuff should have remained?

I think the spam-related stuff should have remained?
Author
Contributor

Probably. My mind just went to "if we're not an auth server, we must be a teen server" and yeeted the branch. will revert.

Probably. My mind just went to "if we're not an auth server, we must be a teen server" and yeeted the branch. will revert.
Tr0n marked this conversation as resolved
Exynox reviewed 2024-04-08 22:48:56 +03:00
@ -1357,3 +1357,3 @@
// 파티 결성 후 충분한 시간이 지나면 경험치 보너스를 받는다.
if (!m_iLongTimeExpBonus && (get_dword_time() - m_dwPartyStartTime > PARTY_ENOUGH_MINUTE_FOR_EXP_BONUS * 60 * 1000 / (g_iUseLocale?1:2)))
if (!m_iLongTimeExpBonus && (get_dword_time() - m_dwPartyStartTime > PARTY_ENOUGH_MINUTE_FOR_EXP_BONUS * 60 * 1000 / 1))
Owner

I suppose we can get rid of the division by 1.

I suppose we can get rid of the division by 1.
Author
Contributor

was unsure if that maybe is used to convert float to int (and if that matters here).

will remove the /1

was unsure if that maybe is used to convert float to int (and if that matters here). will remove the `/1`
Tr0n marked this conversation as resolved
Exynox reviewed 2024-04-08 22:54:34 +03:00
@ -2063,69 +2063,7 @@ teleport_area:
// 3: 이미 같은 이름이 사용중
// 4: 성공
// 5: 해당 기능 지원하지 않음
if ( LC_IsEurope() )
Owner

I don't know how to feel about this. In recent updates, you can now change your name on European servers as well. Should we really delete this feature?

I don't know how to feel about this. In recent updates, you can now change your name on European servers as well. Should we really delete this feature?
Author
Contributor

I will revert the removal. See I haven't played official in so long that I didn't know you can change your name at all.

I will revert the removal. See I haven't played official in so long that I didn't know you can change your name at all.
Tr0n marked this conversation as resolved
Exynox added spent time 2024-04-08 22:59:05 +03:00
2 hours
Tr0n added 12 commits 2024-04-09 18:26:48 +03:00
Tr0n added 3 commits 2024-04-09 19:13:54 +03:00
Exynox reviewed 2024-04-09 19:45:19 +03:00
@ -5442,4 +5264,0 @@
LogManager::instance().CharLog(this, gold, "DROP_GOLD", "");
}
if (false == LC_IsBrazil())
Owner

I just noticed this, the right branch that should've been kept is the one with 150 seconds.

I just noticed this, the right branch that should've been kept is the one with 150 seconds.
Author
Contributor

You're right. Gotta love when people mix true == IsSomething, false == IsSomething and all the negated variations 😅

You're right. Gotta love when people mix `true == IsSomething`, `false == IsSomething` and all the negated variations 😅
Owner

I know that feel, I just hope I didn't miss any either, my head hurts after reading so much source code xD

I know that feel, I just hope I didn't miss any either, my head hurts after reading so much source code xD
Tr0n marked this conversation as resolved
Owner

I suppose the item->StartDestroyEvent(150); thing in char_item.cpp is the only thing left before I can merge this

I suppose the `item->StartDestroyEvent(150);` thing in char_item.cpp is the only thing left before I can merge this
Tr0n added 1 commit 2024-04-09 21:33:28 +03:00
Author
Contributor

Cool, thanks for the thorough review! If you have anything in particular you would like help with, just let me know.

Cool, thanks for the thorough review! If you have anything in particular you would like help with, just let me know.
Tr0n added spent time 2024-04-09 21:40:46 +03:00
6 hours
Owner

No worries, I also made a discord server, even though there isn't much discussed in there: https://discord.gg/UwR7fXzE

No worries, I also made a discord server, even though there isn't much discussed in there: https://discord.gg/UwR7fXzE
Exynox merged commit 175816c81d into nightly 2024-04-09 21:47:23 +03:00
Sign in to join this conversation.
No reviewers
No Milestone
No project
No Assignees
2 Participants
Notifications
Total Time Spent: 8 hours
Exynox
2 hours
Tr0n
6 hours
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: metin2/server#22
No description provided.