Random->nextSignedInt() returns an unsigned value

[CortexPE]

Issue description

• Expected result: Random->nextSignedInt() should return a signed 32-bit integer (according to docs)
• Actual result: The function returns an unsigned, always-positive value

OS and versions

• PocketMine-MP: 3.0.0+ and 4.0.0+, (possibly earlier versions after 32-Bit PM)
• PHP: 8.0.12
• Using JIT: no
• Server OS: Windows

Plugins

A plugin or a world generator is needed to invoke the function, the following snippet may be used to produce a similar output below:

$rnd = new Random(time());
for ($i = 0; $i < 10; $i++){
    var_dump($rnd->nextSignedInt());
}

Crashdump, backtrace or other files

int(3695939109)
int(3432772977)
int(2405991418)
int(716041031)
int(2458067232)
int(138586938)
int(4146417334)
int(1957814663)
int(3472200452)
int(4260561129)

[dktapps]

⚠️ This issue ticket doesn't have all the required information ⚠️ To make it as quick and easy as possible for maintainers to fix your problem, an issue template is provided which lists certain required information. This includes (but is not limited to):

• PocketMine-MP version (use the version command)
• PHP version
• Minecraft version and/or edition
• List of all plugins used (use the plugins command)

Without this information, your problem cannot begin to be investigated, so the issue will be closed. You may amend this one, or create a new one, providing all the required information using one of the provided issue templates.

Help us to help you by providing the information we ask for.

[CortexPE]

Issue description updated

[dktapps]

this is a tough problem to fix because it will change the outputs of all the generators.

[CortexPE]

this is a tough problem to fix because it will change the outputs of all the generators.

I would suggest providing a LegacyRandom class which explicitly declares in the documentation that it has broken outputs, then the new Random class would simply overwrite the nextSignedInt() method, which should fix the behavior of #4646 as well...

I could also suggest naming them Random32 and Random64 respectively, but it would imply that the former works perfectly as intended and doesn't have broken behavior.

This way, old legacy generators that depend on the old broken behavior wouldn't get broken terrain... Given the proper migration on the code-side. (or insist on renaming the fixed Random class to force a BC-break... for safety)

A more comprehensive change is to do proper world generator versioning to possibly support even the future massive changes to terrain generation

[dktapps]

All of these solutions require some kind of BC break.

[dktapps]

After digging into generator sources it appears that the generator was not affected by this problem at all, due to exclusively using unsigned values.

The only thing this actually has any impact on are the following things:

• https://github.com/pmmp/PocketMine-MP/blob/9c06c1a06f22c63e5418d62dda85a5427e5d093d/src/world/generator/noise/Simplex.php#L85 (no one cares about this anyway, the value is not used)
• https://github.com/pmmp/PocketMine-MP/blob/7fd712c1ff5e33c2df8e888f8b3986947f3f4eb4/src/block/TNT.php#L115 (bias of TNT initial motion)
• https://github.com/pmmp/PocketMine-MP/blob/3b7580688c575b8585bb4f95b23b9691f0a7aca1/src/command/defaults/ParticleCommand.php#L132 (bias in particle positions)

[dktapps]

grepPlugins suggests that impact is similarly negligible to plugins, with only SimpleWarp particles being affected (bias). I am however unable to speak of private plugins.