mbbsemu / MBBSEmu

The MajorBBS Emulation Project is an Open Source, Cross-Platform emulator for easily running The MajorBBS & Worldgroup Modules
https://www.mbbsemu.com
MIT License
132 stars 14 forks source link

[WCCMMUD] Majormud - Exp % miscalculation #528

Closed fletcherm closed 2 years ago

fletcherm commented 2 years ago

Module Information

Experience progress percentage is incorrect

The % of exp you have before next level is incorrect.

Here is an example:

[HP=60]:exp                                                   
Exp: 3642 Level: 3 Exp needed for next level: 24 (3666) [101%]

3642 / 3666 calculates to 0.9934533551554828, so I'd expect this to show up as either 99% or 100% (depending on how the bbs / mud rounds). Certainly not 101%.

To Reproduce

... not easy without leveling up a character. I can work on a more reproducible case, but was crossing my fingers we could try running these numbers through their respective calculation functions and/or instructions and see what pops out.

Expected behavior

The % should show up as 99 or 100%.

Screenshots

image

Here is an mbbsemu screenshot with more context in case it is helpful.

Software Information:

enusbaum commented 2 years ago

The calculations for EXP appear to be using F_LUDIV and F_LXMUL Ordinals

enusbaum commented 2 years ago

Attached is the IDA Disassembly of 1.11p for DOS where EXP is calculated.

cseg29:0C4B public _NEW_CALC_EXP_NEEDED WCCMMUD.DLL.i64.zip

fletcherm commented 2 years ago

Update: I'm experiencing problems obtaining more exp -- the exp prompt tells me I need several thousand more experience to level, but when I kill a monster, the game tells me I am overexperienced for my level and need to go train. Going to train doesn't work because, well, I don't have enough exp.

Since it may be related to this bug, I'll wait for this to be corrected. Then I'll re-asses and create a new item if necessary.

synacktic commented 2 years ago

@fletcherm did you try this with the non Krabby Pattie WCCMMUD.MSG I posted? I'm curious if the altered training and level values are messing things up.

synacktic commented 2 years ago

I'm not seeing any issues here. @fletcherm can we close this?

fletcherm commented 2 years ago

Let's keep it open a bit longer. Let me test again with the new package.

And honestly I'd be pretty surprised if it's broken because I'm using the Krabby package. That package has been passed around for years and behavior this broken would have been noticed by now.

The other thing that would convince me to close this are unit tests using the numbers above that show it behaving correctly.

synacktic commented 2 years ago

You are the worst.:p Make sure you are using the latest code from master too. I used that with the archives I posted and Paladine's exp script to test.

fletcherm commented 2 years ago

Yeah, still no good.

[HP=80]:exp
Exp: 8890 Level: 3 Exp needed for next level: 0 (5866) [153%]

8890 / 5866 = 1.515513126491647, so I'd expect to see 151% or 152%, not 153%.

enusbaum commented 2 years ago

I wrote an integration test to check long integer math for percentage calculation. Everything checks out, so it must be something else:

void EXPORT long_math_test()
{
    long value1;
    long value2;
    long expectedResult;
    long actualResult;

    shocst("LONG Tests", "Running");

    value1 = 5;
    value2 = 2;
    expectedResult = 2;
    actualResult = value1 / value2;

    if(actualResult == expectedResult)
    {
        shocst("5/2==2", "True");
    }
    else
    {
        shocst("5/2==2", "False");
    }

    shocst("Result:", "%lu", actualResult);

    expectedResult = 40;
    actualResult = (value2 * 100)/value1;
    if(actualResult == expectedResult)
    {
        shocst("(2*100)/5==40", "True");
    }
    else
    {
        shocst("(2*100)/5==40", "False");
    }

    shocst("Result:", "%lu", actualResult);

    value1 = 5866;
    value2 = 8890;
    expectedResult = 151;
    actualResult = (value2 * 100)/value1;
    if(actualResult == expectedResult)
    {
        shocst("(8890*100)/5866==151", "True");
    }
    else
    {
        shocst("(8890*100)/5866==151", "False");
    }

    shocst("Result:", "%lu", actualResult);

}

void EXPORT ulong_math_test()
{
    unsigned long value1;
    unsigned long value2;
    unsigned long actualResult;
    unsigned long expectedResult;

    shocst("ULONG Tests", "Running");

    value1 = 5;
    value2 = 2;
    expectedResult = 2;
    actualResult = value1 / value2;

    if(actualResult == expectedResult)
    {
        shocst("5/2==2", "True");
    }
    else
    {
        shocst("5/2==2", "False");
    }

    shocst("Result:", "%lu", actualResult);

    expectedResult = 40;
    actualResult = (value2 * 100)/value1;
    if(actualResult == expectedResult)
    {
        shocst("(2*100)/5==40", "True");
    }
    else
    {
        shocst("(2*100)/5==40", "False");
    }

    shocst("Result:", "%lu", actualResult);

    value1 = 5866;
    value2 = 8890;
    expectedResult = 151;
    actualResult = (value2 * 100)/value1;
    if(actualResult == expectedResult)
    {
        shocst("(8890*100)/5866==151", "True");
    }
    else
    {
        shocst("(8890*100)/5866==151", "False");
    }

    shocst("Result:", "%lu", actualResult);

}

image

synacktic commented 2 years ago

Some funny shit from WG2 and 1.11p: Screenshot from 2022-01-07 22-28-03 Screenshot from 2022-01-07 22-49-43

The first shows that the result for the 101% is the same between systems, but the second is way diff, but the expected XP for the next level is different, so maybe there is a config diff here at play. Which should not be the case assuming they were both done on the new installs I made. shrug

synacktic commented 2 years ago

Worth noting that 8090 / 3666 = 2.424986361, so one would expect 242% not 246%

Which reminds us of: "8890 / 5866 = 1.515513126491647, so I'd expect to see 151% or 152%, not 153%"

Something is up, but I don't think it is the Emu.. that said I feel like some sys commands were behaving differently, so I might want to look at that a bit and see what is up.

enusbaum commented 2 years ago

So this is feeling like it's not a bug in MBBSEmu but potentially a bug within MajorMUD if we're seeing the same miscalculation in WG2 & MBBSEmu. I'm also curious if this is an unintended bug between compilers as the WG3NT version might have handled remainders properly where the older compiler for WG2 did not and the code did not account for that.

synacktic commented 2 years ago

Very possibly, but I want to test a little more and maybe also test the WG3NT version. Will be good to compare all 3 on a number of things.

synacktic commented 2 years ago

If there is a DOS specific error, maybe we can softpatch it. ;)

synacktic commented 2 years ago

Screenshot from 2022-01-08 13-58-55 Here ya go, running the exact same MMUD install, WG 2.0 and MBBSEmu both return the same thing.

enusbaum commented 2 years ago

I guess we can close this as not a bug with MBBSEmu, but a bug with MajorMUD... apparently! 🤣