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

Allow for 32 max collisions in test_body_motion #89517

Merged
merged 1 commit into from
Mar 15, 2024

Conversation

mihe
Copy link
Contributor

@mihe mihe commented Mar 15, 2024

The max_collisions parameter in PhysicsServer3D.body_test_motion, and by proxy PhysicsBody3D.test_move and PhysicsBody3D.move_and_collide, is meant to be capped by the size of the fixed-size collisions array in PhysicsServer3D::MotionResult that's set to the size of PhysicsServer3D::MotionResult::MAX_COLLISIONS, which is currently 32.

This is currently being bounds-checked with the ERR_FAIL_INDEX_V macro when using Godot Physics, which incorrectly leads to an error when passing a max_collisions of 32, as the max_collisions parameter is treated as an index rather than a count.

This PR addresses this by switching to ERR_FAIL_COND_V instead.

@mihe mihe requested a review from a team as a code owner March 15, 2024 14:48
@akien-mga akien-mga added enhancement topic:physics cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release labels Mar 15, 2024
@akien-mga akien-mga added this to the 4.3 milestone Mar 15, 2024
Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Makes sense to me.

@akien-mga akien-mga merged commit 84bdc8d into godotengine:master Mar 15, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

@mihe mihe deleted the 32-max-collisions branch March 15, 2024 16:41
@akien-mga
Copy link
Member

Cherry-picked for 4.2.2.

@akien-mga akien-mga removed the cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release label Apr 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants