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

Add randomDirection and random methods to Vector3 and Quaternion #22494

Merged
merged 5 commits into from
Sep 7, 2021

Conversation

brianpeiris
Copy link
Contributor

@brianpeiris brianpeiris commented Sep 3, 2021

Related issue: #19047

Description

Adds a .randomDirection() method to Vector3 and a .random() method to Quaternion. These methods distribute uniformly on a sphere. Includes basic tests and docs.

@brianpeiris brianpeiris changed the title Add a new Random module in examples with an onUnitSphere function Add a new RandomUtils module in examples with an onUnitSphere function Sep 3, 2021
@WestLangley
Copy link
Collaborator

Instead, I would include this in the core library as Vector3.randomDirection() to complement Vector3.random().

Direction vectors in three.js are always unit vectors.

@brianpeiris
Copy link
Contributor Author

brianpeiris commented Sep 3, 2021

I think the intent from the discussion in #19047 is that RandomUtils would grow to include several other random helper functions that weren't obviously necessary to the core library, or were less generally useful. The community could keep adding to RandomUtils without worrying about bloating the core. There was talk of adding a uniformly-random quaternion function as well.

I don't have a strong opinion either way. This approach seems acceptable to me, especially since it shows up in the docs. But if y'all want me to change it to a method on Vector3, I'm happy to do so.

@WestLangley
Copy link
Collaborator

randomDirection() and randomQuaterion() are useful to be included in core, which was your original intent, and I agree with it.

@Mugen87
Copy link
Collaborator

Mugen87 commented Sep 3, 2021

@mrdoob What is your preference here?

I personally think that having RandomUtils makes it easier for the community to add additional utility functions that might be not appropriate for the core. Considering previous discussions about the core math library, it can be sometimes hard (or impossible) to add something new. Having a module in the examples would mitigate this problem.

@WestLangley
Copy link
Collaborator

RandomUtils makes it easier for the community to add additional utility functions...

There is nothing to put in it now.

Vector3 can be interpreted as a direction ( Vector3.transformDirection() ), so Vector3.randomDirection() is appropriate in core given we already include Vector3().random().

@Mugen87
Copy link
Collaborator

Mugen87 commented Sep 3, 2021

Well, I have no problems adding both methods to the core.

However, there are a lot of interesting random functions inspired by random utility classes of other projects that could be a nice addition for three.js. It would be too bad if adding both methods to the core would prevent us from creating RandomUtils.

@WestLangley
Copy link
Collaborator

WestLangley commented Sep 3, 2021

I do not object to having random utils. I just think the method is deserving of core.

EDIT - ... renamed as randomDirection, and in the documentation you can say is it uniform on a sphere..

@mrdoob mrdoob added this to the r133 milestone Sep 6, 2021
@mrdoob
Copy link
Owner

mrdoob commented Sep 6, 2021

I agree with @WestLangley, adding randomDirection() to Vector3 sounds nice and simple.

@brianpeiris
Copy link
Contributor Author

Alright, thanks for the notes. I've added .randomDirection() to Vector3 and I went ahead and added .random() to Quaternion as well, using the method that was linked in #19047.
(Note @WestLangley I chose .random() on Quaternion, since .randomQuaternion() seemed a little redundant)

@brianpeiris brianpeiris changed the title Add a new RandomUtils module in examples with an onUnitSphere function Add randomDirection and random methods to Vector3 and Quaternion Sep 6, 2021
@WestLangley
Copy link
Collaborator

Hmm... You based this PR on code from another source. What is the quaternion convention in that source: [ x, y, z, w ] or [ w, x, y, z ] ?

Alternatively, did you test your code and does it appear to work?

@brianpeiris
Copy link
Contributor Author

@WestLangley Good call, I think it does use [ w, x, y, z ], based on the math in their description of quaternions http://planning.cs.uiuc.edu/node151.html

I'm pretty sure it doesn't actually matter in this case, since we are producing random orientations, but I'll switch the order anyway, in case I lack some understanding of the effects of getting it wrong.

@brianpeiris
Copy link
Contributor Author

I did also test this code in a separate codepen and it seems to work (regardless of the order of the components of the quaternion!) https://codepen.io/brianpeiris/pen/bGRpxGx

@WestLangley
Copy link
Collaborator

I'm pretty sure it doesn't actually matter in this case, since we are producing random orientations

I'm pretty sure it does matter....

I did also test this code in a separate codepen...

That is not a test. You can't test with points only. Please try with a tiny rectangular geometry offset on the z-axis -- or something similar.

@brianpeiris
Copy link
Contributor Author

@WestLangley Is this the kind of test you had in mind? Maybe I'm misunderstanding, because the component order in the randomization function still doesn't seem to matter. https://codepen.io/brianpeiris/pen/LYLbzOz?editors=0010

@WestLangley
Copy link
Collaborator

@brianpeiris Yes, thank you... Significantly increasing the number of boxes, the distribution does appear to be reasonably uniform. I am not sure why the order of the args does not seem to matter...

/ping @Mugen87 for a second opinion, please.

@Mugen87
Copy link
Collaborator

Mugen87 commented Sep 7, 2021

AFAIK, whether W comes first or last is just a order convention which unfortunately differs from math library to math library. It should have not influence on its semantics. W is always the real part.

@mrdoob mrdoob merged commit 0579cde into mrdoob:dev Sep 7, 2021
@mrdoob
Copy link
Owner

mrdoob commented Sep 7, 2021

Thanks!

@WestLangley
Copy link
Collaborator

WestLangley commented Sep 7, 2021

It should have not influence on its semantics. W is always the real part.

@Mugen87 This is not about semantics. The computed real part must be assigned to the proper component.

I was hopeful that you would be able to confirm that the PR is correct after the ordering was changed in 0a5f490.

@Mugen87
Copy link
Collaborator

Mugen87 commented Sep 7, 2021

It is correct.

@brianpeiris
Copy link
Contributor Author

brianpeiris commented Sep 7, 2021

Thanks for the verification and the merge.

Out of interest, I think the component order certainly matters to quaternions when you're doing operations like multiplication, but in this particular case, the math is agnostic.

w = sqrt(1 - u1) * sin(2 pi u2)
x = sqrt(1 - u1) * cos(2 pi u2)
y = sqrt(u1) * sin(2 pi u3)
z = sqrt(u1) * cos(2 pi u3)

It's clear that the w and x components are complements of each other (one being sine and the other cosine) and can be swapped without affecting the distribution, and the y and z components are similarly complements of each other. I suppose what's surprising is that you can swap x and y and still not break the randomization, but perhaps that's accounted for by the fact that they are all related by u1, and thus are all complements of each other (either u1, or 1 - u1). So I think swapping the components will certainly give you different sequences of random quaternions, given the same sequence of random inputs, u1, u2, and u3, but they are ultimately all the same uniformly distributed set of quaternions.

I've graphed it out for fun. It might help illustrate the relations: https://www.desmos.com/calculator/vt0sov9yfi

@brianpeiris brianpeiris deleted the examples-math-random branch September 7, 2021 16:42
@WestLangley
Copy link
Collaborator

@brianpeiris Actually, you did not "swap" a pair of values. You originally assigned every value to the incorrect component.

@brianpeiris
Copy link
Contributor Author

Ah, that's true, but I suppose if my reasoning is correct, it still wouldn't matter that all the components were shifted. Anyway, just wanted to explain my understanding. If there's reason to believe it's still wrong, I'd be interested to hear it.

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.

4 participants