Asking for feedback: tried to get the logic out of Tick()

MyCody · December 1, 2023, 3:32am

Looking for general feedback as well as specific feedback on event handling. My 2 delegates, OnTriggerMeshBeginOverlap and OnTriggerMeshEndOverlap, are just one liners that call UpdateIsActivatedState(). UpdateIsActivatedState() is where all the logic got moved to. If I could eliminate the two delegates and just call UpdateIsActivatedState() that would be more readable. However, the two delegates have different signatures and I don’t think I can overload UFUNCTIONS. Let me know if you have thoughts. Additionally, I modified some of the logic at the end, Let me know if it looks more readable.

// .h 

	/**
	 * Modifies IsActive based on actors tagged with "TriggerActor"
	 * @param bIsActivatedLocal The requested activation state
	 */
	UFUNCTION()
	void UpdateIsActivatedState(bool bIsActivatedLocal);

	/** just calls UpdateIsActivatedState() */
	UFUNCTION()
	void OnTriggerMeshBeginOverlap(UPrimitiveComponent* OverlappedComponent,
	                                 AActor* OtherActor,
	                                 UPrimitiveComponent* OtherComp,
	                                 int32 OtherBodyIndex,
	                                 bool bFromSweep,
	                                 const FHitResult& SweepResult
	){ UpdateIsActivatedState(true); };

	/** just calls UpdateIsActivatedState() */
	UFUNCTION()
	void OnTriggerMeshEndOverlap(UPrimitiveComponent* OverlappedComponent,
	                               AActor* OtherActor,
	                               UPrimitiveComponent* OtherComp,
	                               int32 OtherBodyIndex
	){ UpdateIsActivatedState(false); };

// .cpp

APressurePlate::APressurePlate()
{
	TriggerMesh->OnComponentBeginOverlap.AddDynamic(this, &APressurePlate::OnTriggerMeshBeginOverlap);
	TriggerMesh->OnComponentEndOverlap.AddDynamic(this, &APressurePlate::OnTriggerMeshEndOverlap);
}

void APressurePlate::UpdateIsActivatedState(const bool bIsActivatedLocal)
{
	// is there an actor tagged "TriggerActor" on the pressure plate?  Same logic as Kaan.

	// Update IsActivated.  A little different than Kaan.
	if (TriggerActor && !bIsActivated)
	{
		bIsActivated = true;
		OnActivated.Broadcast();
		UE_LOG(LogOfPressurePlate, Log, TEXT("%s is now Active"), *GetActorNameOrLabel());
	}
	else if (!TriggerActor && bIsActivated)
	{
		bIsActivated = false;
		OnDeactivated.Broadcast();
		UE_LOG(LogOfPressurePlate, Log, TEXT("%s is now NOT Active"), *GetActorNameOrLabel());
	}

The original // Update IsActivated

		if (TriggerActor)
		{
			if (!Activated)
			{
				Activated = true;
				GEngine->AddOnScreenDebugMessage(-1, 1.0f, FColor::White, TEXT("Activated"));
				OnActivated.Broadcast();
			}
		}
		else
		{
			if (Activated)
			{
				Activated = false;
				GEngine->AddOnScreenDebugMessage(-1, 1.0f, FColor::White, TEXT("Deactivated"));
				OnDeactivated.Broadcast();
			}
		}

beegeedee · December 3, 2023, 6:57pm

In my career, I developed 3 rules that helps me reach the correct answer almost 100% of the time.

These are:

Simple is good
If it ain’t broke don’t fix it
Just because you can, doesn’t mean you should

The premise is you ask the 3 questions (do these rules apply) and if the answer is not favourable, you shouldn’t do what you are planning.

So in the original code, rules 1 and 2 are fine and rule 3 applies.

Should you refactor the code?

Now take the code you’ve shared. IMO it fails on the first rule. What you’ve done is not simple and therefore is less maintainable but also for others to work on your code is more difficult too.

Rule 3 also applies. Just because you can do this refactor doesn’t mean you should.

This would generally be my thinking process.

Don’t get me wrong, having code in tick is not perfect but it is simple and easy to follow. Adding to a function called from tick helps.

What the code now is, is 2 delegates being bound for overlap (which BTW runs in a tick like function in the engine, except now you’re monitoring 2 of them) to call a function and that function is essentially what was in the tick. You have to create 2 functions to bind to, and they call a third adding 2 additional layers of code complexity.

This is the way I would look at the code and I hope this thought process makes sense to you.

I never replied sooner as I was trying to figure out if your code made more sense or if would consider doing it this way myself.

rclafferty · December 10, 2023, 1:25pm

I would STRONGLY disagree with this advice.

Rule #1: Perhaps not simple, but it’s simple enough.
Rule #2: The code shown in the lecture is trying to do what’s called “polling”, so it’s arguably broken out the gate. @MyCody has fixed this by moving to a event-based system.
Rule #3: Doesn’t apply (See #2)

Any chance you get to move from polling to an event-based system is ALWAYS a good thing. UE already supports OnBeginOverlap and OnEndOverlap, and is designed for this exact situation, so why not utilize it instead of reinventing the wheel? Hence: YES you should refactor. Honestly, I’m surprised this approach wasn’t discussed in the videos thus far.

Side note: I imagine @MyCody and I are at the same spot in the course at time of our respective comments. I haven’t fully finished watching the session at the moment to see the rest of the logic.

Source: I have 1 year of AAA game dev + many years as an indie dev and got this beat into me pretty quick in AAA. Any time my stuff remotely looked like polling, it got scrutinized.

beegeedee · December 10, 2023, 2:12pm

This is interesting. 1 year is not a lot of time and in fact the company I work for doesn’t consider a developer anything other than a graduate for 2 years after they join. I have spent more time than that on a single project in my current job and my previous job most of the projects span 30 years. I’ve been working in game development for 7+ years and have well over 20 years of C++ experience with commercial work in VR and AR going back 6 years.

My colleague at work is one of the world leading C++ developers who is involved in the latest C++ standards and has worked with AAA companies for, I figure, 20 years give or take.

So forgive me for having experience and knowledge that you perhaps were unaware of. The UE event system is great and I’m not saying the solution is bad, I’m saying it is complicated and given that most taking the course are beginners, simple is indeed a good thing.

With that in mind, his solution is fine but complete overkill for this course given the target audience. Choose to do it however you wish but please do not presume that your solution is better for everyone.

rclafferty · December 10, 2023, 2:23pm

The point of this whole line of questioning is why the videos did not move this to an event-based system. In Blueprint, you ABSOLUTELY would’ve done this, so why not in C++?

“Overkill” is recreating the wheel for no reason. @MyCody had the simpler solution.

RE beginners in the course: Why not bring this up as an alternative and compare the two approaches in the video? This was a missed learning opportunity and a chance to improve the code.

beegeedee · December 10, 2023, 2:54pm

The simple answer to why is it wasn’t necessary. As for recreating the wheel, that is a total stretch. Adding a bit of code to a tick is not recreating the event system. It s far more valuable a lesson to learn how to do something without the use of a black box. The event system (overlaps etc) hide a lot of the logic away. It’s the same reason the training curve for a college graduate takes 2 to 3 years because at the college they teach the black box way of doing things but not how things actually work. Most people I’ve trained over the last 10 years barely knew what binary was, or how a tree works or a list. Getting back to basics and doing things this way is actually incredibly valuable.

As for not showing how to do this with events, the instructor may have considered it but chose not to. His decision. Not yours, not Cody’s. His.

I consider this end of discussion on this matter.

rclafferty · December 10, 2023, 3:20pm

I’d like to know Kaan’s input on this topic. Is there a way to tag him here so he can contribute?

beegeedee · December 10, 2023, 3:33pm

Just so you know, I am also a moderator. Flagging the response from a TA is considered aggressive and inappropriate and such behaviour is bannable - part of the T&C you agree to when signing up to the community. This is your final warning. Next time you will be banned.

I will ask Kaan about it.

beegeedee · December 10, 2023, 9:50pm

Response from the instructor:

using tick is the simpler solution. And also given what we are trying to do, it is enough.

There was more and the gist was best practice is subjective which you could read as in his professional opinion, what is presented is best suited for this course.

Ultimately, the instructors are teaching the courses here, have often been teaching for a number of years and all have industry experience so when they teach something a certain way it is because:
a. they know the target audience
b. they know how to teach
c. they know the subject matter
d. they know the best way to present it to cause the minimum of confusion

I hope this is a satisfactory answer.