Fix buffer overflow in 2D BVH #53230
Conversation
pouleyKetchoupp
added
bug
topic:core
topic:physics
cherrypick:3.x
|
I notice also that in BTW It will work even if the first choice is "wrong" because it will try the other axes. But I have a vague memory that empirically the results were counter-intuitive here, so it may have been intentional. |
core/math/bvh_debug.inc
Outdated
| @@ -22,7 +22,7 @@ String _debug_aabb_to_string(const BVHABB_CLASS &aabb) const { | |||
| sz += itos(-aabb.neg_max.z); | |||
| sz += ") "; | |||
|
|
|||
| Vector3 size = aabb.calculate_size(); | |||
| Point size = aabb.calculate_size(); | |||
| float vol = size.x * size.y * size.z; | |||
There was a problem hiding this comment.
Presumably this bit below won't work either for 2d, as there is no size.z. This function is likely not compiled in and only used for debugging .. it's not absolutely necessary to have it 2d / 3d aware.
There was a problem hiding this comment.
Good catch! It shouldn't be too hard to fix, just for the sake of having a 2D version of the debug stuff.
|
I'm not absolutely sure this is correct because there's an order[3] which is set earlier in the function, which assumes 3d (so order [1] may be nonsense in 2d). I'll have a proper look tomorrow. |
Ah yes, you're right. I missed that min and max are not set to order[0] and order[1] so it doesn't work in 2d. I'll try something and you'll check if you want to do a better fix tomorrow. |
|
Just to make it clear what the function is doing .. when splitting a leaf (with say 256 objects) it is choosing an axis to split into 2 leaf nodes (a and b) and the groups a & b are the objects that will be placed in the respective leaf a & b. So it first tries what it thinks is the most likely axis for splitting. If it doesn't split group a & b equally enough, it then tries the other axes, and chooses the best of the three. If all of them fail, it has a fallback at the bottom. So in the case of 2d, there are only 2 axes, so if it doesn't work for the first, there is only one more to check, and if those fail, the fallbacks. |
Some areas of code were missed and assumed Vector3.
pouleyKetchoupp
force-pushed
the
fix-2d-bvh-overflow
branch
from
135c18d
to
d3c6395
Compare
|
Thanks! |
|
Just to remind, this needs cherry picking / converting to 3.x I think because the issue was in 3.x. |
|
Cherry-picked for 3.4. |
akien-mga
removed
the
cherrypick:3.x
Some areas of code were missed in #48314 for 2D support and still assumed Vector3.
@lawnjelly I searched for other places which refer to Vector3 or iterate over 3 coordinates and didn't find any other problematic cases (with the exception of convex culling which is meant to be supported only in 3D). But it would be good to double check on your side whenever you have time.
Fixes #53203