engine
engine copied to clipboard
playcanvas
Ammo tests
Implements tests for Ammo.js
Moved from #5106, use IDL from playcanvas/ammo.js, put as draft waiting for current IDL used by editor or update to latest kripken/ammo.js.
I confirm I have read the contributing guidelines and signed the Contributor License Agreement.
I wanted to check in on this PR - it's been open for a while! Can I ask where test\ammo.mjs came from? Did you make that yourself by hand? Also, is there any reason not to run tests using the 'real' ammo.js? I'm concerned a stubbed ammo will get out of sync with the actual version of ammo we take from kripken's repo.
I made it myself yes. The initial idea was to replace only few functions however if you declare it once it fires from everywhere so I had to add all functions, which is also a good indicator of which are supported and which are not, especially since the version of Ammo being used is not up to date with kripken's repo (or wasn't last time I checked)
I also have absolutely no background knowledge or skills about tests and it was suggested by @LeXXik in #5106 to do so, and because they seem more aware than me in the matter I went ahead and made that. Honestly I wouldn't mind using Ammo directly either however since it requires the version to be synced it would be a risk as well and mislead users if a contributor add something working only with the latest Ammo version while this version is not the available one in projects from the editor.
The stubs allow to verify that the engine is calling the correct Ammo methods, correct amount of times and passing correct value types. It is not possible to verify those using an actual Ammo library.
The latest updates on your projects. Learn more about Vercel for Git ↗︎
| Name | Status | Preview | Comments | Updated (UTC) |
|---|---|---|---|---|
| engine | ✅ Ready (Inspect) | Visit Preview | 💬 Add feedback | May 14, 2024 2:45pm |
![vercel[bot] avatar](https://avatars.githubusercontent.com/in/8329?v=4 "Published by a pub.dev verified publisher"){kind=small}
@willeastcott what is the status of this ticket? I want to write some features to collision component, but still can't write tests.
I gotta say, I'm still really struggling to see the justification for merging this. Should we be writing stubs for Draco, Basis and whatever other WASM libraries we add in the future? If we merge this, we're essentially committing to maintaining it.
I'm going to summon @slimbuck and @mvaligursky because I really feel like I need other opinions here.
Draco, Basis and whatever other WASM libraries we add in the future?
Yep, we should. Although, I don't think we need to stub everything. Only the methods that the engine is calling should be enough.
The goal of the test is to verify that the engine is calling the correct Ammo methods, correct amount of times and at the right time. It is not possible to verify it with original library (it won't tell us). Stubs serve that purpose - we don't care about Ammo internal functionality during a test, but we (should) care we call proper methods.