Add control to play specific sequence-variations.

Mugen87 / three-m2loader

three.js loader for importing M2 assets from World of Warcraft.

MIT License

25 stars 8 forks source link

Add control to play specific sequence-variations. #15

Closed Mat2095 closed 8 months ago

Mat2095 commented 8 months ago

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 (https://github.com/georgealways/lil-gui/issues/86).

Mat2095 commented 8 months ago

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 commented 8 months ago

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.

Mat2095 commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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 a7b4c206d116eb545196e85e5c6503a203e53cf8 now since I need this for testing 😅 .

Mat2095 commented 8 months ago

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