Respawning Player

[pastel timber]

I’m working on a Unity project and implemented a respawn system in my GameManager script. It handles respawning the player at the start position or the last activated checkpoint. Below is the relevant part of my code.

I’d really appreciate any feedback on:
Code structure and clarity.
Best practices or improvements I might have missed.

[SerializeField] private float respawnDelay;
private Vector3 initialSpawnPosition; // Store the original start position
private Vector3 currentRespawnPosition; // Keep track of where to respawn the player

private void Awake()
{
    if (instance == null)
        instance = this;
    else
        Destroy(gameObject);

    initialSpawnPosition = playerPrefab.transform.position;
    currentRespawnPosition = initialSpawnPosition;
}

public void UpdateRespawnPosition(Transform newRespawnPoint)
{
    currentRespawnPosition = newRespawnPoint.position;
}

public void RespawnPlayer() => StartCoroutine(RespawnRoutine());

private IEnumerator RespawnRoutine()
{
    yield return new WaitForSeconds(respawnDelay);

    GameObject newPlayer = Instantiate(playerPrefab, currentRespawnPosition, Quaternion.identity);
    player = newPlayer.GetComponent<Player>();
}

[pastel knoll]

2 seconds in, your Awake method makes no sense

[pastel timber]

Is it bad?

[bitter shard]

is that some DDOL stuff? (don't even remember how it works but I have seen something similar written in some DDOL tutorial)

[pastel knoll]

Read your code

[bitter shard]

I think what they are doing is destroying an object that is automatically created in the scene load in case the object exists on DDOL scene already. Not that it seems very reasonable way but I think I have seen this way in some old Brackeys video. I would definitely not include DDOL objects in any scenes and just Instantiating if it doesn't exist rather than Destroying always it does

[pastel knoll]

not just that, do you really think that calling Destroy(gameObject) will stop the execution of the current method?

[bitter shard]

Doesn't really make a huge difference though, just destroying the objects each scene loads is just weird to begin with

[pastel knoll]

makes a big difference, I'm pretty sure that the last 2 lines of code in the Awake method should only be run once, when instance is initially null

[bitter shard]

Those are private fields so they would get Destroyed together with the object no?

[pastel knoll]

no, actually, you are correct, not instance set values

still bad code practice

[bitter shard]

that is true. Return should be called aftter Destroy. I don't see DontDestroyOnLoad being called from this code, I assume it's called from some other script on the object, is that the case @pastel timber?

[pastel timber]

I hope this is what you're asking about. This is the DestroyObject file.

using System.Collections;
using System.Collections.Generic;
using UnityEngine;

public class DestroyObject : MonoBehaviour
{
    public void DestroySelf() => Destroy(gameObject);
    
}

[bitter shard]

What I'm asking is do you use DontDestroyOnLoad?

[pastel timber]

I don't think I did.

[bitter shard]

What is the Awake then doing here?

Why are you Destroying youself if instance != null

This is not how Singleton is usually implemented

If you used DontDestroyOnLoad (DDOL), it would make sense but otherwise that seems totally unnesessary line

[pastel timber]

I'm destroying the object if instance != null because it means there’s already an existing GameManager. The reason I’m doing this is to ensure that there’s only one GameManager at a time. If another instance of the GameManager tries to create itself, it would cause issues, so I destroy it to prevent duplicates. The main GameManager will persist across scenes, and any extra ones are removed to keep everything running smoothly.

[bitter shard]

"will persist across scenes" definitely sounds like DDOL

[haughty mirage]

Hey @pastel timber,

Like the others have mentioned (quite a bit), using the singleton pattern is usually considered a “code smell” because it is a bit of a lazy pattern in lieu of more "sophisticated" architecture. But you see it a lot in game development because we’re equally of the mindset of “If it works, it works”.
So I would say it’s fine as long as it’s a conscious choice you’re making. For a game manager I'd also think it's perfectly reasonable.. But keep in mind that if your game manager destroys itself when there are duplicates, you are actively obscuring any cases that might arise where you have, or create, multiple game managers- where it might even be preferable to get errors that make it clear that something is going wrong.

My only points I’d add would be in terms of clarity;
UpdateRespawnPosition could take a Vector3 instead of a Transform. That just makes it more clear that you’re passing in the new point rather than say, a Transform whose position you want to “Update” to move to the respawn position.

RespawnPlayer could also take a parameter for the delay, since it’s otherwise obscured from the caller, who doesn’t necessarily realize that there is a respawnDelay added internally.
This could also be an optional overload, but I would personally expect that if I call RespawnPlayer(), it would respawn the player immediately and not after some delay.

Other than that I'd say it looks really good. I'm not sure why the two very aptly named variables need to be commented when the rest of the code doesn't, but that's more of a style preference I suppose.