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

Port mainstream patches #52

Closed
JakMobius opened this issue Feb 6, 2025 · 4 comments · Fixed by #53
Closed

Port mainstream patches #52

JakMobius opened this issue Feb 6, 2025 · 4 comments · Fixed by #53

Comments

@JakMobius
Copy link

This version of Box2D is affected by flyover#79. b2BroadPhase.UnBufferMove does not remove some proxy moves resulting in assertion failure in b2BroadPhase.UpdatePairs. The suggested fix is to port flyover@48e167c.

@JakMobius
Copy link
Author

JakMobius commented Feb 6, 2025

Also, I suggest considering the rest of the patches as well. It seems like Box2D has received plenty of updates in the last three years.

@Lusito
Copy link
Owner

Lusito commented Feb 14, 2025

I switched mainstream to the original (Box2D from Erin Catto).
The last time I merged changes from it, was june 2024, so that should be up to date (aside from the Switch to Version 3).

This particular code snippet, I must have overlooked when comparing the code with the original. I'll supply a fix for that.

I've skimmed over the changes from flyover and the only ones that seemed like they have not been refactoring/cleanup commits or upstream changes (from Original), which I've done myself already, are these:

flyover@1d2cc3a#diff-eb7280cee2bd76f87794feeb11aba32b708b60cebb310977a8f1f07f69189477

flyover@c9bf588#diff-b7b2e775ba814d459e85a01d20a5c04cfa9f38ff82380d9bd887dbe798f6d22f

And they seem more like features and changes that to not relate to the Original. Not sure if I want to port those changes. Feel free to offer reasons.

@JakMobius
Copy link
Author

I don't insist on porting these changes; it was just a general suggestion. Since you had this bug in your codebase, I thought you were using a three-year-old version.
It's interesting how you ported all the fixes except for this specific one. Back in 2021, I contributed to fixing the same bug in the original Box2D. I was surprised to find the same bug here. Isaac didn't use my pull request back then, so I'm glad to have another opportunity to formally become a contributor. Hope you approve!

@Lusito
Copy link
Owner

Lusito commented Feb 14, 2025

The way I compare changes with the c++ version requires high attention. I guess I must have thought, that this was an improvement for JS that flyover added to avoid going over all elements. I didn't realize, that a proxy could exist multiple times in the array.

I've added a review to your PR. When that is cleared, I'll merge. Thanks!

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 a pull request may close this issue.

2 participants