[cpp] Increase cap for exp/cp bonus.

[shigukahz]

Changed int16 for cap (subpower) to int32 to allow for higher bonuses to be applied to experience items, or even with using !addeffect.

I affirm:

• I understand that if I do not agree to the following points by completing the checkboxes my PR will be ignored.
• I understand I should leave resolving conversations to the LandSandBoat team so that reviewers won't miss what was said.
• I have read and understood the Contributing Guide and the Code of Conduct.
• I have tested my code and the things my code has changed since the last commit in the PR and will test after any later commits.

What does this pull request do?

This change allows values higher than 32k to be placed into the subpower for the status effect commitment and dedication. Doesn't effect the actual max exp you can earn in one go, just how many bonus points you can get per effect application.

Steps to test these changes

Tried to add either effect while int16 was the cap, anything above 33k crashed map and client. Changed to int32 and retried the process, no crash. Once I established it wasn't crashing I applied commitment with power 500 and subpower 99999. Killed enough mobs to simulate the bonus of 99999 to confirm it was applied.

[Xaver-DaRed]

If I may ask, what would be the use-case for this? In other words, is there any particular reason to change this value other than to allow non-retail/custom behabior to happen, or is there an actual use for it?

[shigukahz]

If I may ask, what would be the use-case for this? In other words, is there any particular reason to change this value other than to allow non-retail/custom behabior to happen, or is there an actual use for it?

I'm unaware of any full retail reasoning, would mostly be just something for server owners to be able to change without crashing their server, without having to write a module for it. The only use for it is to allow subPower on those 2 effects to exceed the normal 30,000.

[zach2good]

Get/SetSubPower() is defined as:

uint16 CStatusEffect::GetSubPower() const
{
    return m_SubPower;
}

void CStatusEffect::SetSubPower(uint16 subPower)
{
    m_SubPower = subPower;
}

If your goal is to store more in subpower and then have the maths here support that, then subpower also has to change.

The truncation happens here:

dedication->SetSubPower(cap -= bonus);

Where cap is originally int16 (the subpower) and bonus is int32, but SetSubPower takes a uint16. There's a terrible amount of mixing of widths and signedness's here. I'd work on getting all of those in a row, rather than just boosting the width of cap, since it will get immediately truncated on the way into SetSubPower.

When this is eventually sent to the client, it's done through:

CMessageCombatPacket::CMessageCombatPacket(CBaseEntity* PSender, CBaseEntity* PTarget, int32 param0, int32 param1, uint16 messageID)
...
    ref<uint32>(0x10) = param0;

So this ending up as int32 is fine.

[shigukahz]

So even though the change I made works for raising the subpower for those two effects overall in long run it will cause problems even for those two, unless

uint16 CStatusEffect::GetSubPower() const
{
    return m_SubPower;
}

void CStatusEffect::SetSubPower(uint16 subPower)
{
    m_SubPower = subPower;
}

the uint16 in here matches the other changes made to subpowers around the src? I can't imagine anywhere subpower will ever exceed uint16 let alone int16, other than those two effects so if more needs changing just to apply a quality of life on the exp bonuses, it seems a bit un worth.

[zach2good]

The point I was getting at is that m_SubPower and its getters/setters are all handling it as a uint16. It's being captured currently into an int16, and your change is just extending the range of that int16 to int32. GetPower is also uint16.

[WinterSolstice8]

I'm not sure this is a QoL thing, it looks like you've stumbled on a legitimate bug like Zach mentioned, this function has some suspicious looking wrapping issues,

Untested, but the exact if statement needs an audit, something like this:

diff --git a/src/map/utils/charutils.cpp b/src/map/utils/charutils.cpp
index b5c8c64ddb..086366c3f4 100644
--- a/src/map/utils/charutils.cpp
+++ b/src/map/utils/charutils.cpp
@@ -4730,10 +4730,13 @@ namespace charutils
         if (PChar->StatusEffectContainer->GetStatusEffect(EFFECT_COMMITMENT) && PChar->loc.zone->GetRegionID() != REGION_TYPE::ABYSSEA)
         {
             CStatusEffect* commitment = PChar->StatusEffectContainer->GetStatusEffect(EFFECT_COMMITMENT);
-            int16          percentage = commitment->GetPower();
-            int16          cap        = commitment->GetSubPower();
-            rawBonus += std::clamp<int32>(((capacityPoints * percentage) / 100), 0, cap);
-            commitment->SetSubPower(cap -= rawBonus);
+            float          percentage = static_cast<float>(commitment->GetPower() / 100.f);
+            int32          cap        = static_cast<int32>(commitment->GetSubPower()); // Upcast from uint16 to avoid underflow
+
+            rawBonus += std::clamp<int32>(std::floor(capacityPoints * percentage), 0, cap);
+
+            // Make sure to clamp back down in range of uint16
+            commitment->SetSubPower(std::clamp<int32>(cap -= rawBonus, std::numeric_limits<uint16>::min(), std::numeric_limits<uint16>::max()));

[shigukahz]

I'm not sure this is a QoL thing, it looks like you've stumbled on a legitimate bug like Zach mentioned, this function has some suspicious looking wrapping issues,

Untested, but the exact if statement needs an audit, something like this:

diff --git a/src/map/utils/charutils.cpp b/src/map/utils/charutils.cpp
index b5c8c64ddb..086366c3f4 100644
--- a/src/map/utils/charutils.cpp
+++ b/src/map/utils/charutils.cpp
@@ -4730,10 +4730,13 @@ namespace charutils
         if (PChar->StatusEffectContainer->GetStatusEffect(EFFECT_COMMITMENT) && PChar->loc.zone->GetRegionID() != REGION_TYPE::ABYSSEA)
         {
             CStatusEffect* commitment = PChar->StatusEffectContainer->GetStatusEffect(EFFECT_COMMITMENT);
-            int16          percentage = commitment->GetPower();
-            int16          cap        = commitment->GetSubPower();
-            rawBonus += std::clamp<int32>(((capacityPoints * percentage) / 100), 0, cap);
-            commitment->SetSubPower(cap -= rawBonus);
+            float          percentage = static_cast<float>(commitment->GetPower() / 100.f);
+            int32          cap        = static_cast<int32>(commitment->GetSubPower()); // Upcast from uint16 to avoid underflow
+
+            rawBonus += std::clamp<int32>(std::floor(capacityPoints * percentage), 0, cap);
+
+            // Make sure to clamp back down in range of uint16
+            commitment->SetSubPower(std::clamp<int32>(cap -= rawBonus, std::numeric_limits<uint16>::min(), std::numeric_limits<uint16>::max()));

Oh I assumed it wasn't a bug just as it was meant to be. This is beyond my scope of knowledge now though, would you like me to close the PR and maybe open an issue so it can be looked at when its time?