aframe-extras icon indicating copy to clipboard operation
aframe-extras copied to clipboard

latest build (as per readme) features an old version of animation-mixer that does not match docs / lacks startFrame

Open kylebakerio opened this issue 4 years ago • 6 comments
trafficstars

The version of animation mixer that one finds at https://cdn.jsdelivr.net/gh/donmccurdy/[email protected]/dist/aframe-extras.js does not have a startFrame property, as you see in the repo's version of that component and read about in the readme for that component.

I ended up just copying the component out myself to get it working in a pinch, but I'm not sure if that's just been recently added and a new release is pending or if some other problem is happening with the dist generation.

You can see this by just going to https://cdn.jsdelivr.net/gh/donmccurdy/[email protected]/dist/aframe-extras.js and doing ctrl+f animation-mixer and looking there, and comparing the schema there with the schema at https://github.com/n5ro/aframe-extras/blob/master/src/loaders/animation-mixer.js.

kylebakerio avatar Mar 29 '21 10:03 kylebakerio

Seconding this, couldn't figure out why the "clampWhenFinished" property wasn't working for me, turns out that's not part of the version in the current build either.

IamcalledPhil avatar Apr 14 '21 23:04 IamcalledPhil

Kyle since you spotted this first do you want to send a PR. I should make an effort to approve it quickly.

n5ro avatar Apr 20 '21 10:04 n5ro

I need to update everything. I did locally, but I got stuck during the testing phase of the update because Aframe Physics System also needs updates to Cannon that I just discovered. So its a situation where I fix Cannon first, or change all the examples to Ammo, or ship all the updates without testing all the examples first. Fixing Cannon or switching all the examples to Ammo are options that will take time, updating everything to the latest versions without having working examples in testing is also problematic. So that is why I recommended that someone write a PR for these things. In the mean time I will try to make progress on these issues.

n5ro avatar Apr 20 '21 10:04 n5ro

I'm glad to do a pr, but I need just the smallest amount of guidance; what would an acceptable or for this issue look like? Are you wanting a readme update, or a commit running the build process, or are your thinking something more in depth?

kylebakerio avatar Apr 21 '21 21:04 kylebakerio

I don't know I was thinking, with a PR, of looking to at least add the missing 'startFrame' and 'clampWhenFinished' code, why would we update the readme at this point? but I would need perhaps to look at all the other updates to the code, and perhaps also this update should be done as a version update, on my local system, with npm, while updating everything else, and then updating the github, instead of going the patchwork route of individual lines of code being edited in a PR. I think I need to allocate more of my time to looking into this first a lot more to really answer your question. I think also that I should just change all the demos to use Ammo in Aframe Physics System for now. Because I will need to spend extra time testing the Cannon updates that I have planned.

n5ro avatar Apr 23 '21 09:04 n5ro

and perhaps also this update should be done as a version update, on my local system, with npm, while updating everything else, and then updating the github,

Yeah, it seems to me like a build script needs to be run and a version released. The docs seemed to match the code in this repo's source, but the live version released on CDN's doesn't match the code here in the repo--I only looked for the issue I wanted, haven't looked for the clamp feature but pretty sure I remember seeing that there as well.

I was offering to update the readme to explain the current situation, or to have the docs match the current release, as one option, just so they aren't out of sync.

kylebakerio avatar Apr 24 '21 14:04 kylebakerio

Yes README should be updated to specify the correct urls, created an issue for that #395

vincentfretin avatar Dec 07 '22 12:12 vincentfretin