Comrades!
I was recently spelunking through UE4’s source code as I often do, when I stumbled upon this confusing little piece of code in the Timeline.cpp SetPlaybackPosition() function. The confusing part is present in all of the property access here, but I will just deal with float properties for simplicity.
if( FloatEntry.FloatCurve && (FloatEntry.InterpFunc.IsBound() || FloatEntry.FloatPropertyName != NAME_None || FloatEntry.InterpFuncStatic.IsBound()) )
{
// Get float from func
const float Val = FloatEntry.FloatCurve->GetFloatValue(Position);
// Pass float to specified function
FloatEntry.InterpFunc.ExecuteIfBound(Val);
// Set float property
if (PropSetObject)
{
if (FloatEntry.FloatProperty == NULL)
{
FloatEntry.FloatProperty = FindField<UFloatProperty>(PropSetObject->GetClass(), FloatEntry.FloatPropertyName);
if(FloatEntry.FloatProperty == NULL)
{
UE_LOG(LogTimeline, Log, TEXT("SetPlaybackPosition: No float property '%s' in '%s'"), *FloatEntry.FloatPropertyName.ToString(), PropSetObject->GetName());
}
}
if (FloatEntry.FloatProperty)
{
FloatEntry.FloatProperty->SetPropertyValue_InContainer(PropSetObject, Val);
}
}
// Pass float to non-dynamic version of the specified function
FloatEntry.InterpFuncStatic.ExecuteIfBound(Val);
}
Now I am going to go ahead and parse a few points out that I think will help illuminate my confusion.
This has been a cause of frustration within my team, there is a log spam case that appears both inefficient and in some cases entirely unnecessary. Here is what I see…
if( bool && (redundantbool || FloatEntry.FloatPropertyName != NAME_None || redundantbool) )
{
// Set float property
if (MaybeCheckThisLater?)
{
if (FloatEntry.FloatProperty == NULL)
{
FloatEntry.FloatProperty = FindField<UFloatProperty>(PropSetObject->GetClass(), FloatEntry.FloatPropertyName);
if(FloatEntry.FloatProperty == NULL)
{
UE_LOG(Obnoxious log that has nothing to do with if this timeline calls its delegates at all! Its even called on a tick, yay!);
}
}
if (FloatEntry.FloatProperty)
{
FloatEntry.FloatProperty->SetPropertyValue_InContainer(PropSetObject, Val);
}
}
}
There is even a default case of the function that sets the interp function and curve that declares PropertyName as none, intending it to be optimized to ignore if there are in fact no bound properties…
// TimelineComponent.h
AddInterpFloat(UCurveFloat* FloatCurve, FOnTimelineFloat InterpFunc, FName PropertyName = NAME_None);
So here is my suggestion, why not just
if( FloatEntry.FloatCurve )
{
// Get float from func
const float Val = FloatEntry.FloatCurve->GetFloatValue(Position);
// Pass float to specified function
FloatEntry.InterpFunc.ExecuteIfBound(Val);
// Pass float to non-dynamic version of the specified function
FloatEntry.InterpFuncStatic.ExecuteIfBound(Val);
// Set float property
if (PropSetObject)
{
if (FloatEntry.FloatProperty)
{
FloatEntry.FloatProperty->SetPropertyValue_InContainer(PropSetObject, Val);
}
else if(FloatEntry.FloatPropertyName != NAME_None)
{
FloatEntry.FloatProperty = FindField<UFloatProperty>(PropSetObject->GetClass(), FloatEntry.FloatPropertyName);
if (FloatEntry.FloatProperty)
{
FloatEntry.FloatProperty->SetPropertyValue_InContainer(PropSetObject, Val);
}
else
{
UE_LOG(LogTimeline, Log, TEXT("SetPlaybackPosition: No float property '%s' in '%s'"), *FloatEntry.FloatPropertyName.ToString(), *PropSetObject->GetName());
}
}
}
}
The redundant checks may still be of value for an early out, I get that. But the way I see it if you are going to check 2 out of 3 things that need checking again, you may as well either just go for all 3 without any redundancy. Or just go for all 3 and call it good, If you are going to do an early out for one expensive function you may as well at least check if a string is empty to avoid wasting time and spamming a log.
I think you could even do a check at the time of binding in the AddInterpFloat() function and throw a much less spammy error, and save time on checks later. An early out for the early out if you will.
Seems sensible right? I am still pretty noob in C++ so take this with a grain of salt, but if someone knows why this is the way it is, could you please enlighten me?