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 control to play specific sequence-variations. #15

Merged
merged 3 commits into from
Jan 1, 2024

Conversation

Mat2095
Copy link
Contributor

@Mat2095 Mat2095 commented Dec 31, 2023

The version of lil-gui in three/addons is too outdated. It is 0.17.0, but at least 0.19.0 is needed to allow for updating the options of a drop-down (georgealways/lil-gui#86).

@Mat2095
Copy link
Contributor Author

Mat2095 commented Dec 31, 2023

I guess there are some things you could argue about, for example I chose to disable the variation-control when only one variation exists. This makes it clear to the user that the concept of different variations exists, but not for this specific sequence. But you could find arguments for hiding the variation-control as well, so feel free to veto my decision ^^

@Mugen87
Copy link
Owner

Mugen87 commented Jan 1, 2024

Slightly updated the code but I think this should be okay now!

I like the idea of enabling/disabling multiple variations depending on availability. Since there is in M2 terms always a 0 variation, I think this is better than completely hiding the field.

@Mugen87 Mugen87 merged commit 81a6eae into Mugen87:main Jan 1, 2024
2 checks passed

// TODO: Add support for sequences with external animation data (.anim files)

console.warn( 'THREE.M2Loader: Sequences with external animation data not yet supported.' );
Copy link
Owner

Choose a reason for hiding this comment

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

For simplicity, I have moved the M2_SEQUENCE_EMBEDDED_DATA check to playSequence(). Meaning the manager now reports all sequences and variations again but warns the users if animation data are not available.

@Mat2095
Copy link
Contributor Author

Mat2095 commented Jan 1, 2024

Looks good, but two thoughts:

  • Are you sure for every sequence the first variation has id 0? The variations are not in an array, but each defines its own id, so in theory the list of variations could be [2, 5]. Haven't seen this though, so I guess it's okay.
  • In listVariations you removed the compareFn-argument when sorting the result list. The default in JS is so sort alpha-numerically, so [1, 5, 10].sort() results in [1, 10, 5], so I would create a PR to bring back compareNumber if that's okay.

@Mugen87
Copy link
Owner

Mugen87 commented Jan 1, 2024

Are you sure for every sequence the first variation has id 0?

The default variation index should always be 0. So I guess this is safe.

The default in JS is so sort alpha-numerically, so [1, 5, 10].sort() results in [1, 10, 5], so I would create a PR to bring back compareNumber if that's okay.

That would be indeed good! I've missed the alpha-numerically thing.

@Mugen87
Copy link
Owner

Mugen87 commented Jan 2, 2024

The default in JS is so sort alpha-numerically, so [1, 5, 10].sort() results in [1, 10, 5], so I would create a PR to bring back compareNumber if that's okay.

That would be indeed good! I've missed the alpha-numerically thing.

Actually I have fixed it in a7b4c20 now since I need this for testing 😅 .

@Mat2095
Copy link
Contributor Author

Mat2095 commented Jan 3, 2024

Oh, okay. Sorry I didn't get to it earlier :/

@Mat2095 Mat2095 deleted the show_variations branch January 10, 2024 22:39
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.

2 participants