New shuffle method is not correct

[garyw]

The new shuffle method uses a common algorithm that is, unfortunately, incorrect.

 

See:

https://stackoverflow.com/questions/962802/is-it-correct-to-use-javascript-array-sort-method-for-shuffling

 

https://stackoverflow.com/questions/2450954/how-to-randomize-shuffle-a-javascript-array

 

http://www.robweir.com/blog/2010/02/microsoft-random-browser-ballot.html

 

 

 

[Shrug ¯\_(ツ)_/¯]

I’m positive Jack @GreenSock is aware of the Fisher-Yates Knuth Shuffle (and similar). Maybe there was a reason for not utilizing it, so lets wait to hear from him. Good point though. ?

[GreenSock]

Well, I certainly wouldn't all it "incorrect". My goal was to use the minimum amount of kb and still get randomized shuffling and it does that. Using a different algorithm will roughly triple the amount of kb used for that method. I suppose in the grand scheme of things, it's really not THAT much more in terms of its ratio to the whole GSAP file, but I like being efficient wherever I can. In my tests, the shuffling delivered perfectly acceptable results. If you're looking at millions of elements, sure, it may not be PERFECTLY randomized in a completely unbiased way but when it comes to animations I really don't think it's noticeable. 

 

That being said, I'm open to switching algorithms if the current one is problematic for a real-world situation. Do you have anything like that which you could share? 

[garyw]

 This is what I use:

 

function shuffle(array)
{
    var result = Array.prototype.slice.call(array);    // this method supports nodelists, too.

   for (var i = result.length - 1; i > 0; --i)
    {
        var j = Math.floor(Math.random() * (i + 1));

       var temp = result;
        result = result[j];
        result[j] = temp;
    }

    return result;
}

 

[GreenSock]

Yeah, here's what I whipped together for if I end up switching: 

let shuffle = a => {
  for (let j, v, i = a.length; i; j = ~~(Math.random() * i), v = a[--i], a[i] = a[j], a[j] = v);
  return a;
}

Which is still certainly more verbose than: 

let shuffle = a => a.sort(() => .5 - Math.random())

Again, if anyone has a real-world scenario where that 2nd one doesn't deliver desirable results in an animation context, I'd love to see it. 

[garyw]

I'm not trying to convince you one way or another, but I would just like to add this:

 

If you put a general-purpose utility method in a toolbox, you don't know what people are going to use it for. It could end up being used for a number of non-animation purposes, and nobody will question whether they are getting an even distribution of randomness until somebody else notices it. 

 

<soapbox>If code size is the primary concern, my personal opinion is that the shuffle method should be removed (unless it is required by something else) rather than return mediocre results. </soapbox>

[OSUblake]

On 1/30/2020 at 9:26 AM, garyw said:

The new shuffle method uses a common algorithm that is, unfortunately, incorrect.

 

I'm not sure an algorithm can be "incorrect". More like, can an algorithm be wrong for a given use? ?

 

That said, I think gsap's current shuffle method is perfectly fine for its given use. I know the difference is only a couple of lines of code, which is pretty small, but I'd recommend keeping it as is. It's short, simple, and gets the job done.

 

If someone has a problem with shuffle's sort bias, then I will kindly show them a better way, just like I did with some of gsap's random functions.

 

 

 

[Shrug ¯\_(ツ)_/¯]

On 1/30/2020 at 3:16 PM, GreenSock said:

if the current one is problematic for a real-world situation. Do you have anything like that which you could share? 

 

I think it would still be very applicable to actually see an example(s) from @garyw.

 

 

[garyw]

I posted one above.

[Shrug ¯\_(ツ)_/¯]

Hi @garyw,

 

Glad you responded, but what you posted are discussions about various other functions. What I think people instead wanted to see are examples using the current function which clearly demonstrate how and why you feel its inadequate.

 

Do you have any actual CodePen examples that clearly show the current function that express your point of view? Its up to @GreenSock to make the switch in this case, but I’m sure he wants to see examples from you demonstrating your point clearly and concretely.

 

Also GSAP allows function based values, so people can also easily use whatever logic they desire. I think its important for everyone to look at all the new GSAP 3 Utility Methods as “ease of use helper functions”. They should work in most situations and provide many new capabilities, but they may or may not be all encompassing for everyone or every situation. If that becomes the case anyone can instead utilize GSAP’s function based capabilities or create a plugin to extend things however they see fit based upon specific user logic.

 

Shrug ¯\_(ツ)_/¯

[GreenSock]

5 minutes ago, Shrug ¯\_(ツ)_/¯ said:

Do you have any actual CodePen examples that clearly show the current function that express your point of view? Its up to @GreenSock to make the switch in this case, but I’m sure he wants to see examples from you demonstrating your point clearly and concretely.

Yes, exactly. If there is clear evidence that the current method is inadequate for real-world animation scenarios, I'm happy to make the change. I just wanted to see a practical use case, not Stack Overflow posts about theoreticals.