Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CI: Dynamically replace Math.random() with seeded random. #19549

Closed
wants to merge 1 commit into from
Closed

Conversation

mrdoob
Copy link
Owner

@mrdoob mrdoob commented Jun 4, 2020

This PR replacesMath.random() with a deterministic random dynamically when loading each example. The library's internal usage of Math.random() no longer interferes.

/ping @munrocket

@mrdoob mrdoob added this to the r118 milestone Jun 4, 2020
@mrdoob
Copy link
Owner Author

mrdoob commented Jun 4, 2020

This will not work when we start using vector.random() but should do the trick for now.

@gkjohnson
Copy link
Collaborator

gkjohnson commented Jun 4, 2020

I don't believe this will work, either, because Math.random is used in the js files whereas this fix only replaces the html content. See LightingStrike.js, MeshSurfaceSampler.js, Ocean.js, etc.

If UUID generation is a problem specifically because the amount of times it gets called internally changes frequently then maybe just inserting something like this at the top of <head> is good enough? It would enable use of Vector.random too.

<script type="module">
    import { MathUtils } from 'https://unpkg.com/three/build/three.module.js';
    MathUtils.generateUUID = function() {
        // return a uuid
    };
</script>

@munrocket
Copy link
Contributor

munrocket commented Jun 4, 2020

Yeah, Vector.random will not covered in this case.

We can replace

import * as THREE from '../build/three.module.js';

with

import * as THREE from '../build/three.module.js';
THREE.MathUtils.generateUUID = function() {
     // return a uuid based on window.crypto.getRandomValues()
};

This will work. But limit us in code style in examples.

So I am propose a trick with environment variables for rollup

"build-test": "cross-env TEST=true rollup -c utils/build/rollup.config.js"

and then replace math.random() * 0xffffffff to crypto.getRandomValues(new UInt32Array(1))[0] in generateUUID() function.

Edit: with special build we can face with another problem: we need to store it in another place and replace a link to build in each example like in this PR.

@munrocket
Copy link
Contributor

@mrdoob checked locally, this examples fails

webgl_geometry_extrude_shapes webgl_gpgpu_water webgl_instancing_scatter

because we not cover this functions

THREE.MathUtils.randFloat( - 50, 50 )
jsm/math/SimplexNoise.js
jsm/math/MeshSurfaceSampler.js

seems that we can modify response in puppeteer in this way we don't need additional build.

@munrocket
Copy link
Contributor

Thanks for inspiration, here good one: #19557

@mrdoob mrdoob closed this Jun 4, 2020
@mrdoob mrdoob deleted the e2e branch June 4, 2020 23:16
@mrdoob mrdoob removed this from the r118 milestone Jun 9, 2020
@munrocket munrocket mentioned this pull request Jun 5, 2022
21 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants