MrUnbelievable92/MaxMath

Random8() always returns 255

CodeSmile-0000011110110111 opened this issue · 2 comments

I noticed something odd about Random8 when using the "parameterless" constructor. It always outputs 255 because the internal State remains 0. Interestingly, the Assert in NextState() isn't triggered either, probably because it's a Unit test assertion and not a Debug.Assert() (which, honestly, I would avoid using in Burst compiled code but rather check for Burst Safety Checks = On).

Because of the ctor default value - and because you cannot implement a truly parameterless ctor in a struct - I believe that using a single parameter with a default value in a struct ctor leads to the code actually calling the internal parameterless ctor of the struct when no input value is supplied. It does NOT call the ctor with a default value parameter!

Coincidentally, I cannot "step into" the new Random8() line at all, this only works when supplying a parameter ie new Random8(1).

Random8 works fine when using any value in the constructor, including the default: 0b0111_1001

Here's the code I've tested it with (outside Jobs, not Burst compiled):

var rand = new MaxMath.Random8();
for (int i = 0; i < 10; i++)
{
	Debug.Log(rand.State);
	Debug.Log(rand.NextByte());
	var color = rand.NextByte4();
	Debug.Log(color);
}

Using Unity 2022.1.14f1 with Burst 1.7.4, Mathematics 1.2.6, MaxMath 2.3.0.

I forgot: the fix should be to provide a static "Create" method that calls the ctor with a default value. Remove the default value of the ctor. Implement a proper check in NextState() that catches State==0 and outputs a corresponding error message (ie "use Random8.Create() or provide a seed value").

Default constructors for structs in C# is a feature that will be supported with C#10. I was just following the Unity.Mathematics.Random implementation, though, which has a similar constructor. Unity expects users to know not to have a seed of 0, which this library does, too, throwing an exception otherwise.

Regarding the assertion: They are, first off, part of C# Dev Tools and thus DEBUG (the preprocessor directive) only assertions, but can also be turned off directly in the source or within the Unity Editor under "Window/C# Dev Tools/Manage Saftey Checks...", which yields several advantages over a standard throw method at the cost of more abstract error messages.
The assertions were accidentally turned off with the previous release. I'd suggest turning them on in the editor or updating C# Dev Tools to this fixed version.

Regarding a static "Create" method, which doesn't exist in the Unity.Mathematics API by the way, is already implemented as RandomX.New.