InteractWithCombat mouse check should be done outside of foreach loop

Unity Courses Talk
, ,
#1

As per title, in this chapter you explain the InteractWithCombat method, you cast a ray to see which targets are hit and then if you find any CombatTarget, you check the mouse to see if you have to perform anything and that’s all.

Then why not moving the

if (Input.GetMouseButton(0))

before all the rest of the method code? we need to execute something only if we click on a target, so checking first if the user clicked the mouse button, will save us from wasting CPU for nothing

Another observation is that, personally, I’d write the foreach without checking if null and then doing continue but checking if different from null then attacking and then exiting the foreach with break

Basically my InteractWithCombat is

private void InteractWithCombat()
        {
            if (Input.GetMouseButton(0))
            {
                Ray ray = Camera.main.ScreenPointToRay(Input.mousePosition);
                RaycastHit[] hits = Physics.RaycastAll(ray);

                foreach (var hit in hits)
                {
                    CombatTarget target = hit.transform.GetComponent<CombatTarget>();
                    if (target != null)
                    {
                        fighter.Attack(target);
                        break;
                    }
                }
            }
        }

P.S.
I’m saying this not to be fussy, but to emphasize to those who are reading and want to learn how to program well, that it’s not enough to write code “that works”, but you have to pay attention to the consequences of the code we write (especially in video games where the performance it is important and therefore avoiding running unnecessary code is to be kept in mind)

I know this code may be changed in next chapters, but every chapter should be self contained in terms of code or at least should be explained something like “we could have done things differently but we are changing the code in next chapters”

2 Likes
Too much method extraction imo
#2

Edit: … and in the next lesson we prepare the code for cursor affordance.
Also if we move GetMouseButton outside of the for loop we break that logic, so better to wait
for the end of a complete section of the course before messing too much with the code :slight_smile:

And furthermore, we raycast twice: once for combat, once for movement.

I don’t know if it will change later, but this was handled in a more interesting way in the first version of the course, with the CameraRaycaster component using delegates to notify if you hit an enemy or a walkable layer. But back then there was a raycast every update even if you don’t click, to handle the change of cursor image base on where the mouse point. Probably not the most efficient way neither :slight_smile:

#3

Interesting @MithrilMan I was thinking the same and strangely my code ended up exactly the same as yours, including with the != null and break. Perhaps it’s true we’ll need to change it later but up until now I think the way we did it is the most efficient.

Edit: Indeed 1 minute later in the next lecture it’s explained why it was done the way it was. But I agree entirely that this should be mentioned beforehand that it’s being done temporarily in an inefficient manner because we specifically want to do something later. Omitting saying that leads to confusion. But otherwise this course is the best in all of Unity I’ve taken so far.

Privacy & Terms