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

Ammo tests #5322

Draft
wants to merge 43 commits into
base: main
Choose a base branch
from
Draft

Ammo tests #5322

wants to merge 43 commits into from

Conversation

MushAsterion
Copy link
Collaborator

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.

MushAsterion and others added 27 commits February 27, 2023 13:40
Co-authored-by: Martin Valigursky <59932779+mvaligursky@users.noreply.github.com>
@MushAsterion MushAsterion marked this pull request as draft May 12, 2023 09:47
@GSterbrant GSterbrant added the open source contribution Issues related to contributions from people outside the PlayCanvas team label May 12, 2023
@MushAsterion MushAsterion added the area: physics Physics related issue label May 12, 2023
MushAsterion and others added 2 commits August 17, 2023 15:48
Co-authored-by: Will Eastcott <will@playcanvas.com>
@willeastcott
Copy link
Contributor

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.

@MushAsterion
Copy link
Collaborator Author

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.

@LeXXik
Copy link
Contributor

LeXXik commented Oct 31, 2023

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.

Copy link

vercel bot commented May 14, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: physics Physics related issue open source contribution Issues related to contributions from people outside the PlayCanvas team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants