Ammo tests #5322
Ammo tests #5322
Conversation
Co-authored-by: Martin Valigursky <[email protected]>
GSterbrant
added
the
open source contribution
Co-authored-by: Will Eastcott <[email protected]>
|
I wanted to check in on this PR - it's been open for a while! Can I ask where |
|
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 ↗︎
|
|
@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. |
Yep, we should. Although, I don't think we need to stub everything. Only the methods that the engine is calling should be enough. |
|
Should we use the wasm libraries themselves instead of stubs? |
|
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. |
| _constructor: (x, y, z, w) => { | ||
| if (x !== undefined) { | ||
| assertNum(x); | ||
| assertNum(y); | ||
| assertNum(z); | ||
| assertNum(w); | ||
| } | ||
| }, |
There was a problem hiding this comment.
I know we do call new Ammo.btQuaternion() without arguments, but technically this is just wrong if you take a look at the actual WebIDL:
https://github.com/kripken/ammo.js/blob/1ed8b58c7058a5f697f2642ceef8ee20fdd55e10/ammo.idl#L62-L64
So in a way if there are actual errors, this won't find it, because you just assume that as things are right now is the definition of "correct", even though there may be bugs?
Sorry for stupid question, but I just still don't understand what this really does or how this helps to find bugs in Ammo?
There was a problem hiding this comment.
This doesn't suppose to find bugs in Ammo. Ammo handles its own tests, we don't care about it. This can catch bugs, like the one we had recently, where we were adding a body to dynamics world twice, or an earlier one, where we were destroying the same body twice, etc. This allows to make sure the engine calls correct Ammo methods, correct amount of times and in correct order. What happens inside Ammo is of no importance, thus we stub those.
There was a problem hiding this comment.
About quaternion - we should init those with zeros, as per API. If we don't, those are initialized with garbage, so unless we immediately set values for all components right after, it can introduce hard to debug bugs.
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.