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

anim-controller: fix transitions sorting / minimize code duplication #4057

Merged
merged 7 commits into from
Mar 2, 2022

Conversation

kungfooman
Copy link
Collaborator

A sort function doesn't act on true/false, so this never sorted anything (copy/paste test for F12/DevTools):

transitions = [
  {priority: 10},
  {priority: 50},
  {priority: 1},
  {priority: 2},
  {priority: 100},
  {priority: 3},
];
transitions.sort(function (a, b) {
    return a.priority < b.priority;
});
console.log(transitions.map(_ => _.priority));

Result: [10, 50, 1, 2, 100, 3] (its fixable by replacing < with -)

And then priority sorting is used in multiple places, so this PR reduces the code size/repetition of the same sorting function by just reusing functions (imo saving time and hopefully less bugs)

I confirm I have read the contributing guidelines and signed the Contributor License Agreement.

src/core/sort.js Outdated Show resolved Hide resolved
src/core/sort.js Outdated Show resolved Hide resolved
src/core/sort.js Outdated Show resolved Hide resolved
src/core/sort.js Outdated Show resolved Hide resolved
* @returns {number} A number indicating the relative position.
* @ignore
*/
export const cmpPriority = (a, b) => a.priority - b.priority;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does cmpPriority need to be exported? Should it be internal to this module?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just like to keep things public for potential reuse, but technically it can be private (since its not being used elsewhere)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please make this not exported, and we can merge it in for the next release

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll merge this and hide cmpPriority in separate PR.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@willeastcott
Copy link
Contributor

LGTM. However, @mvaligursky and @ellthompson should approve before merging since this affects both animation and graphics subsystems.

@mvaligursky
Copy link
Contributor

I looks great, but I would not mind some simple test of this in the test/core folder

@mvaligursky mvaligursky added the release: next patch Ticket marked for next patch release label Mar 1, 2022
@mvaligursky mvaligursky merged commit d23e9a6 into playcanvas:dev Mar 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: animation Animation related issue area: graphics Graphics related issue bug release: next patch Ticket marked for next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants