[Engine Optimization Request] Proposing "AND" logic gate node optimization:

[Arty-McLabin]

i've noticed a flaw in AND logic gate in blueprints, and proposing an optimisation.

in C++ and raw logics, boolean "&&" (AND) operator, if the first condition (out of 2 or more) is false, the second condition doesn't get checked, because the result is determined as FALSE already.
Unreal Blueprints don't follow this, and
here's why it is bad:

• additional steps get interpreted and checked (very likely calculated in a long process), which can be simply ignored, to make the AND node so much faster.
• leaves a window for runtime error; imagine this case: {hasKnife AND knife->isScary} null reference will occure, even though the execution will be cut off anyway because of first condition state:FALSE.
• ruined logic compatibility with the native C++, in which the previous section example is a natural expression type which programmers use worldwide.
• [4.15+][VERY SERIOUS] logical flow difference between "nativized build" and "not-nativized build". see [details] at the end of the post for more.
  

also:
i assume the same issue occurs with "OR" gate, in which the first condition "TRUEthness" is enough for success. this can be a good player in optimization as well.
thanks for reading.

solution:
please modify AND and OR blueprint nodes to follow C++ optimization tactic above. complexity=VERY EASY

[details]:
bool UKismetMathLibrary::BooleanAND(bool A, bool B)
{
return A && B;
}

nativized build translates blueprint "AND" to the above, which is a regular C++ way (follows the optimization unlike the blueprint). it can cause very serious and hard-to-detect bugs if any of the later conditions (after a FALSE one) contains an impure function ("impure" means it alters some variables states on the run, it other words it is Read+Write), this function will be shot in blueprint but ignored in C++. a direct way to desync hell.
[/details]

[eXi]

I looked into it and as far as I can see the Sequence nodes (such as the OR and AND boolean nodes), work like this:

They construct a comparisong nodes for each two pairs of pins.
And I would guess (no idea here as I couldn't find that), they are processed like this:

(Pin1 && Pin2 = A)
(Pin2 && Pin3 = B)
(Pin3 && Pin4 = C)

(A && B = X)
(B && C = Y)

(X && Y = Z)

And Z is the return value.

It would be solved by making sure that after Pin1 is false, the rest isn't executed anymore, but I don't think that's easily possible.
The example of "if(SomePointer != nullptr && SomePointer->SomeBoolean)" would, in Blueprints, still pass the "SomeBoolean" value to the whole process, which results in accessing the Pointer anyway.

Would like to have some more insight on this from Epic devs though.

[Arty-McLabin]

would, in Blueprints, still pass the "SomeBoolean" value to the whole process

yep. and it's bad for us devs. that's why i propose the ticket

but I don't think that's easily possible.

performing the "check Pin1" step before dereferencing Pin2, shouldn't be hard, as it's an imperative scripting language (actually even on pure-functional it's achievable by lazy evaluation)

[Raildex_]

It's already requested:
https://forums.unrealengine.com/show...-for-Blueprint

[Arty-McLabin]

It's already requested:
https://forums.unrealengine.com/show...-for-Blueprint

thanks, didn't know about it.
it's sad nearly 2 years has passed since then and it is still same, however 4.15 could be a gamechanger, as it adds even more reasons to accept the request

[KrautPotato]

Did someone take a look into the engine source?
Because this

(Pin1 && Pin2 = A)
(Pin2 && Pin3 = B)
(Pin3 && Pin4 = C)

(A && B = X)
(B && C = Y)

(X && Y = Z)

And Z is the return value.

would be pretty dumb to be honest.

e/ Also, a feature / change request should probably be made in the hub, not in the forum.

[eXi]

As far as I know, when compiling the code, it calls "ExpandNode" on the K2Node, or not?
And the one responsible for these sequence nodes is this one:

void UK2Node_CommutativeAssociativeBinaryOperator::ExpandNode(FKismetCompilerContext& CompilerContext, UEdGraph* SourceGraph)
{
	Super::ExpandNode(CompilerContext, SourceGraph);

	if (NumAdditionalInputs > 0)
	{
		const UEdGraphSchema_K2* Schema = CompilerContext.GetSchema();

		UEdGraphPin* LastOutPin = NULL;
		const UFunction* const Function = GetTargetFunction();

		const UEdGraphPin* SrcOutPin = FindOutPin();
		const UEdGraphPin* SrcSelfPin = FindSelfPin();
		UEdGraphPin* SrcFirstInput = GetInputPin(0);
		check(SrcFirstInput);

		for(int32 PinIndex = 0; PinIndex < Pins.Num(); PinIndex++)
		{
			UEdGraphPin* CurrentPin = Pins[PinIndex];
			if( (CurrentPin == SrcFirstInput) || (CurrentPin == SrcOutPin) || (SrcSelfPin == CurrentPin) )
			{
				continue;
			}

			UK2Node_CommutativeAssociativeBinaryOperator* NewOperator = SourceGraph->CreateBlankNode<UK2Node_CommutativeAssociativeBinaryOperator>();
			NewOperator->SetFromFunction(Function);
			NewOperator->AllocateDefaultPins();
			CompilerContext.MessageLog.NotifyIntermediateObjectCreation(NewOperator, this);

			UEdGraphPin* NewOperatorInputA = NewOperator->GetInputPin(0);
			check(NewOperatorInputA);
			if(LastOutPin)
			{
				Schema->TryCreateConnection(LastOutPin, NewOperatorInputA);
			}
			else
			{
				// handle first created node (SrcFirstInput is skipped, and has no own node).
				CompilerContext.MovePinLinksToIntermediate(*SrcFirstInput, *NewOperatorInputA);
			}

			UEdGraphPin* NewOperatorInputB = NewOperator->GetInputPin(1);
			check(NewOperatorInputB);
			CompilerContext.MovePinLinksToIntermediate(*CurrentPin, *NewOperatorInputB);

			LastOutPin = NewOperator->FindOutPin();
		}

		check(LastOutPin);

		UEdGraphPin* TrueOutPin = FindOutPin();
		check(TrueOutPin);
		CompilerContext.MovePinLinksToIntermediate(*TrueOutPin, *LastOutPin);

		BreakAllNodeLinks();
	}
}

I'm not so into the whole Node compilation stuff yet, but the for loop looks like it's creating a new UK2Node_CommutativeAssociativeBinaryOperator for each incrementing pair of Pins from the original UK2Node.
These are also the pins that get "filled" when hitting the "AddPin" slate button on the node.

[Arty-McLabin]

e/ Also, a feature / change request should probably be made in the hub, not in the forum.

you mean github?

[eXi]

you mean github?

Maybe he means answerHUB. You usually get more attention from Epic devs there, but that mostly only counts for bug reports.
Feature request work here too, if put in the correct sub-forum.

You can still do that later, but I think you might want to find out HOW to solve that first :P

[Arty-McLabin]

Maybe he means answerHUB. You usually get more attention from Epic devs there, but that mostly only counts for bug reports.
Feature request work here too, if put in the correct sub-forum.

You can still do that later, but I think you might want to find out HOW to solve that first :P

i've didn't use the forums before, did i post it in the wrong sub forum? where should i move it better in this case?

oh and i'm not a master of blueprint VM source-code, otherwise i would just push the solution to GitHub :P
thanks for cooperation by the way

[BrUnO XaVIeR]

I'm sure I've seen this topic before...
Btw, things like this must be sent to Epic as a pull request on GitHub or they will never see it; the engine ppl aren't fond of reading forums, you only get their attention on GitHub.