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

Remove md5 and use xxHash128 for hashing algorithm in debug infos #297

Merged
merged 3 commits into from
Dec 9, 2024

Conversation

Paulo-21
Copy link
Contributor

@Paulo-21 Paulo-21 commented Dec 4, 2024

#294
Hello, i have choosen xxHash128 which is a non cryptographical hashing function and stay with the lenght of 128 bits to match the previous output with md5.
You can view the difference in term of performance bellow
Before :
https://rapier.rs/demos3d/index.html
https://rapier.rs/demos2d/index.html
After :
https://pi-univers.fr/rapier.js/testbed3d/dist/
https://pi-univers.fr/rapier.js/testbed2d/dist/

Copy link
Contributor

@Vrixyz Vrixyz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, That's great!

I'll give @sebcrozet a chance to review before merging as it touches dependencies and there might be details I'm unaware of.

On a side note, the example pyramid seems to perform worse on your build than on the official one (on my iphone ~28ms vs ~20ms frames), I imagine that's a difference in build settings, but it's an interesting and surprising difference, which may be worth understanding why.

@Vrixyz
Copy link
Contributor

Vrixyz commented Dec 5, 2024

I realize on the website, md5 is advertised, so we might want to update that too for consistency.

But consider this outside of the scope of this PR, as that's outside of this repository and needs decision validation.

@Vrixyz Vrixyz requested a review from sebcrozet December 5, 2024 13:56
Copy link
Member

@sebcrozet sebcrozet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks! Why did the package.json of rapier need to be modified, considering that PR code changes are about the testbed only?

@Vrixyz I'm actually seeing the opposite: their build is slightly faster (on Android though). I think the main factor is that the official demos are still on rapier 0.12 whereas the new build is on 0.14.

@Vrixyz Vrixyz merged commit 5c6f9cd into dimforge:master Dec 9, 2024
@Vrixyz
Copy link
Contributor

Vrixyz commented Dec 9, 2024

Thanks!

@Paulo-21
Copy link
Contributor Author

Hello, does it will be backported to the website demo ?

@Vrixyz
Copy link
Contributor

Vrixyz commented Dec 16, 2024

Hello, does it will be backported to the website demo ?

Eventually, yes 👍 , but there's no strict schedule on doing so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants