Jump to content
Search Community

New shuffle method is not correct

garyw test
Moderator Tag

Recommended Posts

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? 

  • Like 2
Link to comment
Share on other sites

 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;
}

 

Link to comment
Share on other sites

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. 

  • Like 2
Link to comment
Share on other sites

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>

  • Like 2
Link to comment
Share on other sites

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.

 

 

 

  • Like 2
Link to comment
Share on other sites

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 ¯\_(ツ)_/¯

  • Like 2
Link to comment
Share on other sites

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. 

  • Like 1
Link to comment
Share on other sites

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now
  • Recently Browsing   0 members

    • No registered users viewing this page.
×
×
  • Create New...