[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:

• [x] I understand that if I do not agree to the following points by completing the checkboxes my PR will be ignored.
• [x] I understand I should leave resolving conversations to the LandSandBoat team so that reviewers won't miss what was said.
• [x] I have read and understood the Contributing Guide and the Code of Conduct.
• [x] 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.

[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?

[github-actions[bot]]

This PR has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

[Xaver-DaRed]

Abandoned