Deterministic random in UUID

[munrocket]

Follow up discussion in #17949

[Mugen87]

I don't think it's a good idea to make UUID deterministic. This will potentially break applications.

I also don't understand why you change MathUtils.generateUUID() in order to fix the CI. Can you please explain why this is necessary?

[munrocket]

Already explained.

Or you not believe in math or something? We will choose equally good function.

Math.random actually a bad PRNG, but it use time inside.
I think you should explain your position @Mugen87.

[sciecode]

@Mugen87 the idea of using a deterministic random function is to make sure the screenshots used for CI testing are always the same and the tests are actually reliable. But I also don't understand how UUID relates to that, my understanding is that we would need to use a deterministic random function for the cases where the examples rely on Math.random() not on the UUID used internally.

Or you not believe in math or something? We will choose equally good function.

I don't think that is necessary. Mugen asked a straight forward question, there's no need for this sort of stance. We're all trying to work together towards a better library, let's try and be courteous to one another.

[munrocket]

But I also don't understand how UUID relates to that, my understanding is that we would need to use a deterministic random function for the cases where the examples rely on Math.random() not on the UUID used internally.

The reason why UUID can use another random function (or big integer iterator) actually quite simple. For example after refactoring in the core, you create a new textures/meshed that invoke Math.Random() in another sequence. After that we need to recreate 92 screenshots.

We don't have any non deterministic part in the core except this UUID generation. So the easiest way to prevent this in future was this commit. Yes exist a way how to fix it in other style. For example we can use PRNG in all examples directly. But I need to find all the places and fix it. If he meant this, why he didn’t say? Especially when he block the same commit from WestLangley.

We also can proof that this commit with sin() function was not good enough and will cause to collision, because it was based cyclic function with some period P in one random, and P+1 with P+2 will also collide in this situation, because of the nature of this cycles.

Obviously that current situation with refactoring in the core not good and should be fixed. And we should work in collaborative way and give a good feedback, instead of such a dumb questions when he knew what's wrong was with this commit. Straight forward way is to say what's wrong and suggest another way. Right now we talking about all off this, instead of solving problems.

[munrocket]

I am suggesting to use special build of a library with fixed random for testing.
Because we using it in 868 situation and 92 examples.

[peteroupc]

I see several issues:

• Your choice of PRNG is not the best; for example it admits only 2^31 seeds, so that no more than 2^31 values can be chosen at random, whereas UUIDs (version 4) have 122 bits of randomness, meaning that there are considerably more UUIDs possible than the number of seeds in your current PRNG -- namely 2^122 to be exact. On the other hand, other PRNGs with 128 or more bits of state, such as xoroshiro128++, have enough state to choose any UUID at random.
• Similarly, you give seeds as Integers. However, an Integer is, in practice, limited to 32 bits, which may be enough for examples and casual games, but not enough to generate a UUID at random. Given the popularity of three.js, the possibility for collisions becomes extremely likely if UUIDs are generated by a PRNG initialized with a small seed; see "C++ Seeding Surprises" for an example.

[Mugen87]

And we should work in collaborative way and give a good feedback, instead of such a dumb questions when he knew what's wrong was with this commit.

Sorry, but what you are saying is really offensive and I refuse to review code from you any longer.

[munrocket]

@Mugen87 I will glad for that, thanks. This commit is good and solve a problem generally because we need another deterministic function in UUID for testing to prevent such a problem with refactoring in future. But it not fit to production and we can make special build for testing. That's all.

[munrocket]

So, I was right here, you was wrong. Huh?
We need to reopen and refactor this PR for special build.

We already have two PRs in core that damaging 92 screenshot. #19513 #17949

[mrdoob]

@munrocket

Can you please explain what does this PR solve?

Also, when changing generateUUID we do tables of memory and performance changes between browsers, platforms and devices.

You can read more here: #13069

[munrocket]

This PR is not ready for merge, but I am already explained two times, while you guys not and I need to guess what's wrong.

We need two deterministic random functions in tests because simple hook for the Math.Random not enough, also we can't change all 92 example, this is contr-productive and makes examples harder to read.

The idea is to have another seed for random functions in UUID.

[munrocket]

Actually current UUID generator is interesting. We not measured probability of collisions and just trust to 3 built-in Math.Random with some time based hash. BigInt probably better fit, but it's not widely supported.

[mrdoob]

This PR is not ready for merge, but I am already explained two times, while you guys not and I need to guess what's wrong.

Sorry, I don't have time to read the discussion and related discussions. Can you share a link to the post where you explained it?

[munrocket]

#19411 (comment)

[mrdoob]

We need two deterministic random functions in tests because simple hook for the Math.Random not enough

Why is not enough?

[munrocket]

We use window.Math.Random in UUID and same window.Math.Radnom in examples.
Since JS is highly dynamic language I just changing this function in the page, but my implementation based on one global seed. It’s clear?

Give collaborator right back and I will explain 5-th time :D

[makc]

@mrdoob I think he gist of it is, if the screenshot is based on value №123, 124 and 125, and then you make a change in 3js that makes one more uuid to be created, the same example is now based on values 127, 128 and 129. Since Mugen does not want prng in uuid, I suggested getRandomValues for uuid instead

[mrdoob]

@makc Thank you! I understand now.

[mrdoob]

How about doing:

/* Deterministic random */

	let seed = Math.PI / 4;
	window.Math.random2 = function () {

		const x = Math.sin( seed ++ ) * 10000;
		return x - Math.floor( x );

	};

And then dynamically replace all the Math.random() in the page with Math.random2() before taking the screenshot?

Basically, separating the Math.random() used in THREE.MathUtils.generateUUID() from the Math.random()s used in the example.

[munrocket]

Probably will solve a problem but maybe not. The hard part here:

• some of the examples generates new textures/objects on each frame
• don't know when page will be loaded ( when we taking screenshot or validating example )

So I guess this solution will be not robust, since we can't find a moment when all resources created.

[WestLangley]

We need to load all resources prior to rendering once with a deterministic generator. We generally do not write the examples that way.

[munrocket]

Need to try it, it’s easier in implementation.

[munrocket]

Need to try it again, maybe UUID cycled or something. Looks a good hack, actually.

[makc]

@DefinitelyMaybe

language barrier

here is a russian saying for you: после драки кулаками не машут. it was 2 weeks ago, no point to ask that now.

[mrdoob]

How about doing:

/* Deterministic random */

	let seed = Math.PI / 4;
	window.Math.random2 = function () {

		const x = Math.sin( seed ++ ) * 10000;
		return x - Math.floor( x );

	};

And then dynamically replace all the Math.random() in the page with Math.random2() before taking the screenshot?

Basically, separating the Math.random() used in THREE.MathUtils.generateUUID() from the Math.random()s used in the example.

#19549