Question Registering Key Press with 2d Collision

New to unity, been following some basic tutorials, looking at the documentation etc but i have run into slight snag that i cant figure out.

The goal: My player character should be able to interact with an NPC only when they are within the bounds of the second characters 2d box collider by pressing the "space" key. This should hide the player character sprite, disable movement, focus on the NPC sprite and change the animation. Once that animation finishes, it should unhide the Player sprite.

Current progress: The Player is registering in the debug log that they are in collision with the NPC sprite and can be hidden with a press of the "space" key.

Issue: The key press only seems to work infrequently, and only on the edges of the box collider. The key press only appears in the debug log when it triggers correctly and infrequently, even when pressed repeatedly.

Code (CSharp):

1. public class PlayerMovement : MonoBehaviour
2. {
3.  
4.     public Rigidbody2D PlayerRB;
5.     public float Speed;
6.     public float input;
7.     public SpriteRenderer spriteRenderer;
8.     Animator animator;
9.     bool isMoving;
10.    
11.  
12.     private void Start()
13.     {
14.         animator = gameObject.GetComponent<Animator>();
15.     }
16.     // Update is called once per frame
17.     void Update()
18.     {//movement on the X axis
19.         input = Input.GetAxisRaw("Horizontal");
20.         if(input < 0)
21.         {//if moving left, flip sprite and enable "isMoving" switch in animator
22.             spriteRenderer.flipX = true;
23.             animator.SetBool("isMoving", true);
24.         }
25.         else if (input > 0)
26.         {//if moving right, set sprite to original orientation and enable "isMoving" switch in animator
27.             spriteRenderer.flipX = false;
28.             animator.SetBool("isMoving", true);
29.         }
30.  
31.         if(input == 0)
32.         {//if not moving, disable "isMoving"
33.             animator.SetBool("isMoving", false);
34.         }
35.     }
36.      //Detect if character is in the bounds of another collider
37.      private void OnTriggerStay2D(Collider2D collider2D)
38.     {//show debug in log
39.         Debug.Log("Trigger");
40.         //If in bounds of another collider, enable press of "space" key
41.         //if key pressed, hide player sprite
42.         if (Input.GetKeyDown(KeyCode.Space))
43.         {
44.             Debug.Log("Space!");
45.             spriteRenderer.enabled = !spriteRenderer.enabled;
46.         }
47.     }
48.  
49.  
50.      private void FixedUpdate()
51.     {
52.         PlayerRB.velocity = new Vector2(input * Speed, PlayerRB.velocity.y);
53.     }
54. }
55.        

Any help would be very much appreciated, and to be clear, i am only hoping for help with the current issue as i wish to try and figure out any further features myself.

 

OnTriggerStay or any other physics related method like FixedUpdate etc. run in the physics loop. They only get called periodically; they don't get called every frame! That is why it'll work infrequently. https://docs.unity3d.com/Manual/ExecutionOrder.html :



First off, don't add additional responsibility to player movement script. Add the logic of checking if the player is inside the trigger of something, disabling sprites etc. to another script.

Secondly, to solve the problem of infrequency; when the player enters a trigger, enable another script that checks if the space key is pressed inside the Update method and do your logic there. Don't forget to disable the script with the space key logic when the player exits the trigger

Let me know if you need the full example code

 

All of this has been super helpful, i managed to set up a second script to handle collision and call it in the first. The key press is now functioning as frequently as it is pressed. It did however result in the player sprite being hidden whenever the key was pressed, regardless of collision.

To remedy this i have added tags to the game objects "player" for the player and "Client" for the npc.
The goal is to have it so the player sprite will only vanish when the key is pressed in collision with an object marked as "Client"

However i am being thrown an exception:
NullReferenceException: Object reference not set to an instance of an object
Collision.OnTriggerStay2D (UnityEngine.Collider2D collider2D) (at Assets/Scripts/Collision.cs:22)
PlayerMovement.Update () (at Assets/Scripts/PlayerMovement.cs:45)

If This should be put in another thread of its own please let me know, but if not then here is the code from the 2 scripts.

Collision Script:

Code (CSharp):

1. using System;
2. using System.Collections;
3. using System.Collections.Generic;
4. using UnityEngine;
5.  
6. public class Collision : MonoBehaviour
7. {
8.     public SpriteRenderer spriteRenderer;
9.  
10.  
11.     // Start is called before the first frame update
12.     void Start()
13.     {
14.  
15.     }
16.  
17.     //Detect if character is in the bounds of another collider
18.     public void OnTriggerStay2D(Collider2D collider2D = null)
19.     {//show debug in log
20.         Debug.Log("Trigger");
21.         //If in bounds of another collider, enable press of "space" key
22.         if (collider2D.gameObject.tag != "Player")
23.         {
24.            
25.            
26.             //if key pressed, hide player sprite
27.             if (Input.GetKeyDown(KeyCode.Space))
28.             {
29.                     Debug.Log("Space!");
30.                 spriteRenderer.enabled = !spriteRenderer.enabled;
31.             }
32.          
33.         }
34.     }
35.  
36.     // Update is called once per frame
37.     void Update()
38.     {
39.  
40.     }
41.  
42. }

Player Movement Script:

Code (CSharp):

1. using System.Collections;
2. using System.Collections.Generic;
3. using Unity.VisualScripting;
4. using UnityEngine;
5. using UnityEngine.UIElements;
6.  
7. public class PlayerMovement : MonoBehaviour
8. {
9.  
10.     public Rigidbody2D PlayerRB;
11.     public float Speed;
12.     public float input;
13.     public SpriteRenderer spriteRenderer;
14.     Collision collisionScript;
15.     Animator animator;
16.     bool isMoving;
17.    
18.  
19.     private void Start()
20.     {
21.         collisionScript = GameObject.FindGameObjectWithTag("Player").GetComponent<Collision>();
22.         animator = gameObject.GetComponent<Animator>();
23.     }
24.     // Update is called once per frame
25.     void Update()
26.     {//movement on the X axis
27.         input = Input.GetAxisRaw("Horizontal");
28.         if (input < 0)
29.         {//if moving left, flip sprite and enable "isMoving" switch in animator
30.             spriteRenderer.flipX = true;
31.             animator.SetBool("isMoving", true);
32.         }
33.         else if (input > 0)
34.         {//if moving right, set sprite to original orientation and enable "isMoving" switch in animator
35.             spriteRenderer.flipX = false;
36.             animator.SetBool("isMoving", true);
37.         }
38.  
39.         if (input == 0)
40.         {//if not moving, disable "isMoving"
41.             animator.SetBool("isMoving", false);
42.         }
43.  
44.         {
45.             collisionScript.OnTriggerStay2D();
46.         }
47.  
48.     }
49.  
50.  
51.      private void FixedUpdate()
52.     {
53.         PlayerRB.velocity = new Vector2(input * Speed, PlayerRB.velocity.y);
54.     }
55. }
56.  

The Collision script is currently attached only to the "Player" game object

 

you should completely ditch OnTriggerStay

instead use OnTriggerEnter, and you can make a bool "playerInRangeOfClient = true"

use OnTriggerExit to set it to false

Code (CSharp):

1.     using System;
2.     using System.Collections;
3.     using System.Collections.Generic;
4.     using UnityEngine;
5.  
6.     public class Collision : MonoBehaviour
7.     {
8.         public SpriteRenderer spriteRenderer;
9.  
10. public bool playerInRange = false;
11.  
12.  
13.         // Start is called before the first frame update
14.         void Start()
15.         {
16.  
17.         }
18.  
19.         //Detect if character is in the bounds of another collider
20.         public void OnTriggerEnter2D(Collider2D collider2D = null)
21.         {
22.  
23.  
24.             if (collider2D.gameObject.tag == "Player")
25.             {
26.            
27.       playerInRange=true;
28.          
29.             }
30.         }
31.  
32.       public void OnTriggerExit2D(Collider2D collider2D = null)
33.         {
34.  
35.  
36.             if (collider2D.gameObject.tag == "Player")
37.             {
38.              
39.       playerInRange=false;
40.  
41.            
42.             }
43.         }
44.         // Update is called once per frame
45.         void Update()
46.         {
47.  
48.        
49.                 //if key pressed, hide player sprite
50.  
51.   if (playerInRange==true)
52.            
53. {
54.                 if (Input.GetKeyDown(KeyCode.Space))
55.                 {
56.                         Debug.Log("Space!");
57.                     spriteRenderer.enabled = !spriteRenderer.enabled;
58.                 }    }
59.         }
60.  
61.     }
62.  

you should also remove this from your code

Code (CSharp):

1.  {
2.             collisionScript.OnTriggerStay2D();
3.         }

also the script i posted above goes on the client

 

What karderos said is correct; though you should go a step forward and put the update method of Collision class to a new script that is attached to the same gameobject as the Collision script. Instead of using the playerInRange field in the Collision class, you'll just enable or disable the new script that has the Update method. So the new script will look something like:

Code (CSharp):

1.  
2. using UnityEngine;
3.  
4. public class YourScriptName: MonoBehaviour
5. {
6.     void Update()
7.    {
8.        if (Input.GetKeyDown(KeyCode.Space))
9.       {
10.           Debug.Log("Space!");
11.           spriteRenderer.enabled = !spriteRenderer.enabled;
12.       }
13.    }
14. }
15.  

The collision script will just have a

[SerializeField] YourScriptName scriptToEnableOnTrigger;

field that'll be enabled in places where playerInRange is set to true, and disabled in places where playerInRange is set to false.

This style of enabling a script with an update method when certain conditions are met reduces the cpu load(because there is one less Update method running with one less if check in all conditions), increases the ram load just a bit because every class has a bit of overhead

Also dont name anything with the default type names or similar. A class named Collision is terrible naming because there is already a class called Collision. I'd name it something like EnableComponentOnTriggerStay etc.

Btw

In the future, you'll want to just disable collisions between layers to prevent checking for tags inside the collision or trigger callbacks. It'll help the main thread when it executes the physics loop because there'll be less physics checks between objects and there'll be less if statements in the physics methods. You can disable collisions between layers in the collision matrix https://docs.unity3d.com/Manual/LayerBasedCollision.html

 

Thank you all for all the help, i have thinks working in a way im very happy with now!

Ill keep the suggestion's for streamlining in mind, especially the naming conventions.