Looking for a more elegant solution / Hover, select, unselect objects using Raycast

Hi, I am looking for advice from more experienced developers on best practices to reorganize my code.

I wrote a script to create an outline effect when mouse is hovering over the object (achieved changing layers), on mouse click the object is fully highlighted, and when clicked again the object is unselected.

It is working as it's supposed to do, but I feel like using so many IFs in Update is not the best practice. I'd like to hear your opinion on how can this code be improved.

PS I'd prefer to stick to Raycasts (not to use OnMouseOver() and similar functions), because I'd like to adapt this example to controllers in the future.

Code (CSharp):

1.  
2.     Ray ray;
3.     RaycastHit hit;
4.     bool isHit = true;
5.     bool isSelected = false;
6.     int layer_Default = 0;
7.     int layer_Hover = 11;
8.     int layer_Selected = 12;
9.  
10.     void Update()
11.     {
12.         ray = Camera.main.ScreenPointToRay(Input.mousePosition);
13.         isHit = Physics.Raycast(ray, out hit);
14.  
15.  
16.         if (isHit && hit.collider.CompareTag(tag) && Input.GetMouseButtonDown(0))
17.         {
18.             isSelected = !isSelected;
19.             gameObject.layer = layer_Selected;
20.         }
21.  
22.         else if (isHit && hit.collider.CompareTag(tag) && !isSelected)
23.         {
24.             gameObject.layer = layer_Hover;
25.         }
26.  
27.         else if (!isHit && isSelected && Input.GetMouseButtonDown(0))
28.         {
29.             isSelected = false;
30.             gameObject.layer = layer_Default;
31.         }
32.  
33.         else if (!isHit && !isSelected)
34.         {
35.             gameObject.layer = layer_Default;
36.         }
37.     }

 

Coding wise, the only issue here is that you're needlessly evaluating the same conditions multiple times:

Code (csharp):

1.  
2. if (isHit && hit.collider.CompareTag(tag))
3. {
4.    if(Input.GetMouseButtonDown(0))
5.    {
6.        isSelected = !isSelected;
7.        gameObject.layer = layer_Selected;
8.    }
9.    else if(!isSelected)
10.    {
11.        gameObject.layer = layer_Hover;
12.    }
13. }
14.  

... this reads a little better and will be more efficient.

But the big issue is that this script doesn't scale. If you have 500 interactable objects in the scene, you're going to fire 500 raycasts and evaluate 500 different states and layer changes every single frame. What you really need to do is split this logic into two components: one that does the raycast to find if the object is interactable, and one that handles what exactly happens when that interactable is interacted with.

 

Thank you so much for your input! I added an additional condition which will return the object to a default state when unselected (by clicking anywhere in the scene), or if cursor is not over it.

Code (CSharp):

1.  
2. else if (!isHit)
3.         {
4.             if (isSelected && Input.GetMouseButtonDown(0))
5.             {
6.                 isSelected = false;
7.                 gameObject.layer = layer_Default;
8.             }
9.             else if (!isSelected)
10.             {
11.                 gameObject.layer = layer_Default;
12.             }
13.         }

I will definitely have in mind the problem you indicated and I agree. In this scene I'm having a few interactive objects and less than 10 objects in total so I didn't think it would be an issue in this particular case.

 

Technically you could use Unity's EventSystem. and instead of performing the checks on update you have your monobehaviour use the IPointerEnterHandler and IPointerExitHandler and it'll catch it in those methods.

This is the elegant solution. more so cause it separates:

• Responsibilities of the specific highlight behavior
• Managment of the selection state
• Raycasting behaviour
• Input Module which uses the current input devices to provide the info the raycasting behaviour needs.

The Eventsystem means you can have your current script only worry highlighting logic and not controllers or raycasting,. Then when its time to add features for the Controllers you just implement a new input Device handler specifically for Controllers and swap that input module in and the system will work by using that.

Plus while it may not matter in your specific case, the Eventsystem was designed to be able to scale. so its not 500 objects all managing selection state. Its one eventsystem managing selection state (it tracks the "selected" gameobject) and just messages the components that have been Entered/Exited. meaning those 500 objects don't need to constantly check if they are hovered/selected.

 

Thanks a lot for introducing me to this. It looks like a more robust solution for multiple reasons, I'll have to check it out!