Refactoring Code

Unreal Courses Show
,
#1

I’ve taken another extra step to allow the designer more editability when it comes to the door.

void UOpenDoor::BeginPlay()
{
	Super::BeginPlay();
	
	ActorThatOpens = GetWorld()->GetFirstPlayerController()->GetPawn();

	InitialYaw = GetOwner()->GetActorRotation().Yaw;
	CurrentYaw = InitialYaw;
	OpenAngle += InitialYaw; // TargetYaw = TargetYaw + InitialYaw;

	if (!PressurePlate)
	{
		UE_LOG(LogTemp, Warning, TEXT("\"%s\" has DoorComponent, but no pressure plate set."), *GetOwner()->GetName())
	}
}


// Called every frame
void UOpenDoor::TickComponent(float DeltaTime, ELevelTick TickType, FActorComponentTickFunction* ThisTickFunction)
{
	Super::TickComponent(DeltaTime, TickType, ThisTickFunction);
	
	if (PressurePlate && PressurePlate->IsOverlappingActor(ActorThatOpens))
	{
		OpenCloseDoor(DeltaTime, OpenAngle, DoorOpenSpeed);
		DoorLastOpened = GetWorld()->GetTimeSeconds();
	}
	else if (DoorLastOpened <= GetWorld()->GetTimeSeconds() - DoorCloseDelay)
	{
		OpenCloseDoor(DeltaTime, InitialYaw, DoorCloseSpeed);
	}
}

void UOpenDoor::OpenCloseDoor(float DeltaTime, float Yaw, float DoorSpeed)
{
	CurrentYaw = FMath::FInterpTo(CurrentYaw, Yaw, DeltaTime, DoorSpeed); // smooth transition door open
	FRotator DoorRotation = GetOwner()->GetActorRotation(); // create editable variable that has the current door rotation
	DoorRotation.Yaw = CurrentYaw; // set the yaw to whatever the interp function has it set to currently

	GetOwner()->SetActorRotation(DoorRotation); // after all the calculations above, set the door to the current rotation that the lerp is calculating
	return;
}

image
I’ve allowed the ability for the designer to set both open and close speeds all the while using one single function to open and close the door.

If someone could tell me if this is a good idea and if it makes the code more efficient please let me know. I’d like to know if I done a good job re-factoring the code to make it more efficient than it was :slight_smile:

3 Likes
#2

Its fine. I wouldn’t nit pick.

There’s always things that may be done to break the code down more or de-duplicate.

For example, you call GetOwner()->GetActorRotation() twice (once for yaw, once for rotation)

It can be minimized to one small function that gets rotation instead of continually typing GetOwner()->GetActorRotation() you could just type eg GetRotation(). That way too, if UE4 deprecated GetActorRotation() and switched to eg GetRotate() for some weird reason, you wouldn’t have to find all uses of GetActorRotation() and change them. But that’s just one possible reason.

Even more so, it can be split off into anther class just for Actor so that you have Actor related things there.

The most important part though is to make it so that its understood quickly as well as making sure the code works as expected.

For here, what you have is perfectly fine though. I actually did this without refactoring and kept everything required together. That way I can glance at it immediately and know everything that’s required without jumping functions where possible. That allows me to fully understand what’s required in a very short time. I do that because its just learning code.

If you look at code from some popular open source projects, they may have really crazy code jumps all over the place. When finding something that leads to something, it can take a stupid long time to find out where something is coming from. That makes things overly difficult but good for re-usability.

Because if for example, you needed GetActorRotation() in another class then you would need yet another smaller function there or just use your own eg Actor class and then call GetRotation() so that you would never need to duplicate it again and if that Rotation changed then you would go to just one function instead of many.

You don’t need to put any of that into practice now though. Just get used to refactoring while you work on code same as you just did.

Also, as with rotation, you could do a GetYaw() that even more simply then calls custom function eg GetRotation() and gets the yaw thus making that one yet easier still.

#3

Thank for ideas @SamW!
I used separate functions with FInterpConstantTo to “slam” shut. If gives a more ‘slammy’ look which I think can only be fully appreciated when one added sound.

#4

Nice job for explaining why you refactored the way you did!

#5

Looks very sweet. You don’t need "PressurePlate && " in TickComponent(). You have already done the check in BeginPlay().

#6

me too.

Privacy & Terms