Skip to content

Invalid asserts in b2DynamicTree #1084

Description

@MelvMay-Unity

Invalid asserts in b2DynamicTree_SetCategoryBits

b2DynamicTree_SetCategoryBits asserts that the target proxy's children.child1 and children.child2 equal B2_NULL_INDEX. On a leaf node those fields alias userData through a union, so the asserts fail for any leaf whose user data is not 0xFFFFFFFFFFFFFFFF. The function is therefore unusable on any proxy created with real user data in an assert-enabled build.

1. The node layout: one union, two views of the same 8 bytes

include/box2d/collision.h:

typedef struct b2TreeNode
{
	/// The node bounding box
	b2AABB aabb; // 16

	/// Category bits for collision filtering
	uint64_t categoryBits; // 8

	union
	{
		/// Children (internal node)
		b2TreeNodeChildren children;  // <-- child1: bytes 0-3, child2: bytes 4-7

		/// User data (leaf node)
		uint64_t userData;            // <-- the SAME bytes 0-7
	}; // 8

	union
	{
		int32_t parent;
		int32_t next;
	}; // 4

	uint16_t height; // 2
	uint16_t flags;	 // 2
} b2TreeNode;

children.child1 and the low half of userData are the same four bytes. Which member is meaningful depends on whether the node is internal or a leaf.

2. Proxy creation: a leaf stores the caller's user data in that union

src/dynamic_tree.c:

int b2DynamicTree_CreateProxy( b2DynamicTree* tree, b2AABB aabb, uint64_t categoryBits, uint64_t userData )
{
	...
	int proxyId = b2AllocateNode( tree );
	b2TreeNode* node = tree->nodes + proxyId;

	node->aabb = aabb;
	node->userData = userData;                    // <-- writes the union: child1 now reads (int32_t)userData
	node->categoryBits = categoryBits;
	node->height = 0;
	node->flags = b2_allocatedNode | b2_leafNode; // <-- this IS a valid leaf
	...
}

Leaves carrying arbitrary user data is the documented contract, not an accident:

uint64_t b2DynamicTree_GetUserData( const b2DynamicTree* tree, int proxyId )
{
	B2_ASSERT( 0 <= proxyId && proxyId < tree->nodeCapacity );
	return tree->nodes[proxyId].userData;
}

3. The asserts: reading the union through the other member

src/dynamic_tree.c. The declared contract in collision.h is just "Modify the category bits on a proxy. This is an expensive operation." with no precondition on user data:

void b2DynamicTree_SetCategoryBits( b2DynamicTree* tree, int proxyId, uint64_t categoryBits )
{
	b2TreeNode* nodes = tree->nodes;

	B2_ASSERT( nodes[proxyId].children.child1 == B2_NULL_INDEX );        // <-- reads low 32 bits of userData
	B2_ASSERT( nodes[proxyId].children.child2 == B2_NULL_INDEX );        // <-- reads high 32 bits of userData
	B2_ASSERT( ( nodes[proxyId].flags & b2_leafNode ) == b2_leafNode );  // <-- the actual leaf invariant

	nodes[proxyId].categoryBits = categoryBits;
	...

B2_NULL_INDEX is -1 (0xFFFFFFFF), so the two children asserts pass only when userData == 0xFFFFFFFFFFFFFFFF. Every other user data value fails them on a perfectly valid leaf. The third assert (the b2_leafNode flag) is the correct invariant and is the only one of the three that tests what the function actually requires.

4. Minimal repro

b2DynamicTree tree = b2DynamicTree_Create();
b2AABB aabb = { { 0.0f, 0.0f }, { 1.0f, 1.0f } };

int proxyId = b2DynamicTree_CreateProxy( &tree, aabb, 1ULL, /*userData*/ 0ULL );

// child1 aliases userData's low bits: reads 0, not B2_NULL_INDEX (-1).
b2DynamicTree_SetCategoryBits( &tree, proxyId, 2ULL ); // B2_ASSERT fires on children.child1

5. Why it goes unnoticed

Nothing inside Box2D calls b2DynamicTree_SetCategoryBits; it exists purely as public API surface. The broadphase stores shape indices (0, 1, 2, ...) as leaf user data (broad_phase.c), so if Box2D did call the function on its own proxies it would trip its own assert the same way. The asserts only fire the first time an external caller uses the documented API with real user data in an assert-enabled build.

Proposed fix

Delete the two children asserts and keep the b2_leafNode flag assert, which is the real precondition:

void b2DynamicTree_SetCategoryBits( b2DynamicTree* tree, int proxyId, uint64_t categoryBits )
{
	b2TreeNode* nodes = tree->nodes;

	B2_ASSERT( ( nodes[proxyId].flags & b2_leafNode ) == b2_leafNode );
	...

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions