Update AllegianceManager.cs

[rkroska]

uint was limiting to 4.29 billion passup xp even if the flag was off

[gmriggs]

Looks good in theory. I was wondering if there is a test case for this? I assume the player would need to be granted > 4 billion XP from a single kill or quest reward?

[rkroska]

Its more about offline passup than it is a single big item, but I suppose some servers could have something that size? The test case specifically was setting up a vassal and grinding on huge mobs for awhile, then log the patron back on and see that they had more than the int max passup. This is super common on some of the infinite servers or enlightenment situations with super powered vassals. It requires patrons to stay logged in 100% of the time

[gmriggs]

Hmm, I'm not sure I understand then. DoPassXP is not called once when the patron logs in. It is called each time someone earns XP from a single mob kill or quest reward. Even if each individual kill/quest reward is capped at ~4b, those pieces still accumulate in a long.

offline_xp_passup_limit already works as described. Players are able to accumulate > 4b xp when it is disabled. The only thing I see this PR would do would make it so a single kill or quest reward could exceed 4b and pass up correctly. To my current understanding, would require some kind of custom content..

[rkroska]

It's called through HandleAllegianceOnLogin(), -> AddAllegianceXP(), -> GrantXP() ->UpdateXpAllegiance() -> PassXP()->DoPassXP where the change is.
It would never have worked as it appears to be intended since:
patron.AllegianceXPCached = Math.Min(patron.AllegianceXPCached + passupAmount, uint.MaxValue);
would imply that a minimum of either AllegianceXPCached +passupAmount would never be more than uint.MaxValue since the passup amount was a uint and Cached would be empty on patron login. Hence the login XP results in only uint.MaxValue as maximum.
Regardless, even if you don't believe me, this seems like it's a net win with a small fix.

[gmriggs]

AddAllegianceXP() -> GrantXP() ->UpdateXpAllegiance() is not a path that happens.

AddAllegianceXP() calls GrantXP() with ShareType.None. GrantXP() only calls UpdateXpAllegiance() if ShareType has the Allegiance flag, which it does not.

This is easy to test in-game:

• appraise yourself, /setproperty PropertyInt64.AllegianceXpCached 8000000000
• relog
• upon login: 22:34:05 Your Vassals have produced experience points for you.
  Taking your skills as a leader into account, you gain 8,000,000,000 xp.

You can also step through in the debugger and confirm the player is indeed granted 8 billion xp.

We also know this is functional because this was the default behavior in ACE for years, before the offline_xp_passup_limit flag was added. A large # of players were well aware that they could stash more than ~4b of passup xp on ACE servers originally. and many players actually preferred it that way.

It would never have worked as it appears to be intended since:
patron.AllegianceXPCached = Math.Min(patron.AllegianceXPCached + passupAmount, uint.MaxValue);

I'm not sure why that line is in question, since that is the line that is run when offline_xp_passup_limit=true. We are talking about offline_xp_passup_limit=false.

I agree that DoPassXP() shouldn't be truncating to uint, just in case a single kill or quest reward happens to be over 4b xp for custom content. However, the description of the PR that this is a bug that causes offline_xp_passup_limit not to work, is verifiably false.

That is what I was trying to clear up -- I was looking for the actual test cases in order to test and verify this patch

[rkroska]

Ah - got it. I had ShareType and XpType reversed in my read through of GrantXP. That's probably more a "first time here" kind of thing when I tried to trace it through. Now I'm wondering if I had something setup incorrectly in the shard bools settings initially, because you're right I had the breakpoint on the line that should not have been triggering. The ShareType also explains why I couldn't test this with /grantxp command now that I trace that through. Instead I spawned a few dozen custom monsters with tweaked XpOverride.

Anyway, I think you're right now that I understand what ShareType and XpType are doing, not the setting I thought I was fixing but still minor benefit.

[gmriggs]

Yeah the code is a bit confusing around there. As mentioned I think this is still a good change, and that DoPassXP() shouldn't be unintentionally truncating any values > 4b. I just wanted to make sure I fully understood the test cases first.