GetNodeInstance returns the wrong node for TimeLimit decorator

Hi,

We’ve been having problems with decorators not aborting lower priority tasks when they should. After an extensive debugging session we think we’ve found the issue, but are unsure of the solution. We’ll try to describe the the consequence and the source of the problem:

Problem:

In BehaviorTreeInstance::Initialize the line (BehaviorTreeTypes.cpp)

UBTDecorator* InstancedDecoratorOb = Cast(DecoratorOb->GetNodeInstance(OwnerComp, DecoratorMemory));

returns a wrong pointer when DecoratorOb is TimeLimit decorator. Subsequently the child index for this InstancedDecoratorOb is incorrectly overwritten on the next line: (BehaviorTreeTypes.cpp)

InstancedDecoratorOb->InitializeParentLink(DecoratorOb->GetChildIndex());

When the instanced decorator with the incorrect child index then triggers and requests execution the line

RequestedOn->DoDecoratorsAllowExecution(*this, InstanceIdx, RequestedByChildIndex);

in (BehaviorTreeComponent.cpp) then checks the decorators on a different node than the one triggering this check because RequestedByChildIndex is wrong.

Suspected cause

UBTNode::GetNodeInstance seems to make the assumption that if GetSpecialNodeMemory returns a valid pointer that the node itself is instanced. Looking at the implementation of UBTNode::GetSpecialMemorySize() this seems to be a valid assumption, but this method is overridden in UBTAuxiliaryNode in the following manner:

uint16 UBTAuxiliaryNode::GetSpecialMemorySize() const
{
	return bTickIntervals ? sizeof(FBTAuxiliaryMemory) : Super::GetSpecialMemorySize();
}

which invalidates the assumption for AuxiliaryNodes that have bTickIntervals set to true (e.g. BTDecorator_TimeLimit).

Could you confirm this bug? And do you have a suggested fix for this? We are unsure if the caller of GetNodeInstance should be responsible for checking if the node is actually instanced, or that GetNodeInstance should just return nullptr if it is not instanced. Or perhaps we are misreading the intentions of this code?

Best regards,
Steijn

Great find! UBTAuxiliaryNode::GetSpecialMemorySize() was meant to extend memory block with tick intervals data. Without initialization data will be zeroed and potentially give valid object instance which belongs to different node.

UBTNode::InitializeMemory() override shouldn’t touch SpecialMemory block, there’s no guarantee that parent class implementation will be called. Usually it’s not - main reason why part of data is extracted into SpecialMemory block.

I’d say that best place for change would be inside UBTNode::InitializeInSubtree() function. If node doesn’t want instancing ( bCreateNodeInstance == false ), try accessing SpecialMemory block and setting NodeIdx to INDEX_NONE before calling node’s InitializeMemory(). Same order of operations as instancing case.

For now, I’ll enter new bug report, but fix will probably look as described above.

Thank you for your response. We’ve changed the else case of UBTNode::InitializeInSubtree (bCreateNodeInstance == false) to:

		if (FBTInstancedNodeMemory* MyMemory = GetSpecialNodeMemory(NodeMemory))
		{
			MyMemory->NodeIdx = INDEX_NONE;
		}
		InitializeMemory(OwnerComp, NodeMemory, InitType);

which fixed the problem.

I do wonder why UBTNode::GetNodeInstance does not test on bCreateNodeInstance explicitly as its documentation says returns node instance if bCreateNodeInstance was set. I feel a test on this variable would be more explicit and safe some unnecessary work in this function.