I am a recent graduate who is trying to grow my skills in game development through practice and independent personal projects (which I have more time to do now that I am out of school). However, it is likely due to this inexperience that I am hitting a couple frustrating roadblocks.
For my current project, I wanted to implement a Character Selection feature into my game (something that was not taught to us in my studies) with two options for players to choose from to not overcomplicate it. However, this feature has a massive bug, as no matter which character the player picks both appear and overlap each other. I am also having issues with some of the VFX I implemented for certain actions (such as using magic to attack enemies), where the effect and sound are playing constantly rather than be triggered by their corresponding actions. Below is the repository for this project, which I have made public so it may be accessible to provide full context into the current state of the game:
I would greatly appreciate any guidance and/or advice on what to do to resolve this, as I feel at a complete loss in trying to solve it alone with the knowledge I have. I would truly appreciate the advice/feedback/help. Thank you for your time!
While I don’t know this for sure, I think educational programs are usually less about covering specific arbitrary game development problems (e.g. a character selection screen) and more about providing the necessary skills and tools to tackle such problems yourself.
Regarding the project, one issue is that for some it might require installing an editor version they don’t currently have installed. The project can be upgraded of course, but that can sometimes introduce new issues.
I actually did try to look at the project, but it’s very large and took a long time to upgrade. Also, I’m not sure which scene(s) I’m supposed to be looking at.
In any case, the advice you’ll likely get here is to start debugging. Here are some suggestions:
You mentioned multiple bugs/problems. Choose one, and focus exclusively on that until it’s solved.
The project is very large. Try creating a stripped-down version that just demonstrates the specific problem you’re trying to solve, and has an appropriate scene already open and ready to go. If others do want to look at it, that would make it easier for them to do so.
Post screenshots here demonstrating the problem, e.g. the overlapping characters you mentioned. You could also post relevant code if it’s narrow enough in scope, e.g. the character selection code.
I’ll also add the fairly obvious suggestion to check the console for errors and warnings. Not saying you haven’t done that, but sometimes things can be missed. (Sometimes if there are both errors and warnings, it’ll be a warning and not an error that shows up at the bottom of the window, which can maybe mislead one into thinking there are no errors. Anyway, just be sure to check all the console output.)
Bugs are generally fixed by debugging, not by having strangers looking at strange code.
As jyk points out, check the logs first. That’s always your first stop.
Then get to debugging so you can know what is going on.
By debugging you can find out exactly what your program is doing so you can fix it.
Use the above techniques to get the information you need in order to reason about what the problem is.
You can also use Debug.Log(...); statements to find out if any of your code is even running. Don’t assume it is.
Once you understand what the problem is, you may begin to reason about a solution to the problem.
Remember with Unity the code is only a tiny fraction of the problem space. Everything asset- and scene- wise must also be set up correctly to match the associated code and its assumptions.
I didn’t open the project in Unity, but after looking through the code, I noticed several questionable design choices that greatly increase complexity and make the project much harder to debug. Some of the issues that stand out are:
In scripts such as Monsters and Boss , you declare a GameObject variable named self and initialize it with self = this.gameObject; , but its value never changes afterward. Why? This variable serves no purpose. Since you already have a reference to the component, you can access the gameObject property directly. The self variable is completely unnecessary.
There is very little separation of concerns, which makes it difficult to understand the responsibilities of each class. For example, I opened the ProtagMovement class expecting to find movement-related logic, but instead found character-selection logic. Character selection is not something most developers would expect to find in a class responsible for movement.
In ProtagMovement , you also declare a public GameObject self; , but it is never initialized in code. I assume it is assigned through the Inspector. This is inconsistent with the other uses of self in the project, where it is assigned through code.
In the Start method of this class, you have the following code:
if (GetComponent<CharaSelect>().galPicked)
{
if (self.gameObject.name == "MascPlayer") Destroy(self.gameObject); //Destroy unseleccted Player
}
else if (GetComponent<CharaSelect>().guyPicked)
{
if (self.gameObject.name == "FemPlayer") Destroy(self.gameObject); //Destroy unseleccted Player
}
This code destroys the object assigned to self under certain conditions, but why duplicate the logic? Both branches perform the same operation.
Then, in Update , you have:
if (galPicked)
{
if (self.gameObject.name == "MascPlayer") Destroy(self.gameObject); //Destroy unseleccted Player
}
else if (guyPicked)
{
if (self.gameObject.name == "FemPlayer") Destroy(self.gameObject); //Destroy unseleccted Player
}
Do you really need to check whether a GameObject should be destroyed every frame? This is almost certainly incorrect. Once again, both branches perform the exact same operation.
Additionally, what are the galPicked and guyPicked variables in this class? They do not receive their values from the CharaSelect instance whose galPicked and guyPicked values are used in Start. In CharaSelect , these values change based on player input. Here, however, they are simply declared and initialized to false . This leads me to believe that you have two GameObjects using this script, and you manually configure one as the male character and the other as the female character through the Inspector. Then, based on the values in CharaSelect , you destroy whichever character was not selected.
This is unnecessarily complicated. You have to maintain the selected state in one script, configure matching values in the Inspector, ensure the GameObject names (MascPlayer and FemPlayer ) remain correct, and then destroy the object you do not want.
Remove all of this logic and simply create two prefabs. Based on the values of galPicked and guyPicked in CharaSelect , instantiate the appropriate prefab instead.
Some additional notes:
All of your variables use camelCase . While that is a valid naming convention, you do not appear to distinguish between fields, local variables, and other member types nor between accessibility levels. This makes the code harder to read because I frequently had to jump to declarations to understand what a variable represented and where it could be modified.
You have far too many public variables. As with the previous point, this makes understanding and debugging the code significantly more difficult. Unless a variable needs to be accessed by another script, it should be private . If it only needs to be exposed in the Inspector, use [SerializeField].
Along the same lines, if a value only needs to be read from another script, expose it through a property with a public getter and no public setter. This makes it much easier to understand what data can be modified and from where. Right now, virtually everything can be changed from anywhere. I had to use GitHub’s “Find References” feature repeatedly just to determine whether a variable was being used and from where its value could change.
You call GetComponent and FindGameObjectWithTag inside Update . In the Update method of ProtagMovement alone, I counted two calls to FindGameObjectWithTag and three calls to GetComponent . Cache these references once and reuse them instead of performing the lookups every frame.
You have a method declared as: IEnumerable MagicShield(float damage)
which is called like this: MagicShield(protection);
This is incorrect. The method is a coroutine and should be started as one. Also, why is it public if no other script calls it?
These are just some of the issues I noticed while examining the character selection logic in ProtagMovement . I did not review the rest of the project, like the VFX issues.
My advice would be to spend some time revisiting the fundamentals before continuing with a project of this size:
Naming conventions
Understanding when to use private , public , and [SerializeField]
Proper coroutine usage
Instantiating prefabs instead of placing everything in the scene and deleting what is not needed
Caching Find and GetComponent results instead of calling them every frame
Understanding built-in MonoBehaviour properties such as gameObject
Basic architectural principles such as the Single Responsibility Principle and separation of concerns
Without a solid grasp of the fundamentals, debugging will quickly become a nightmare. As the project grows, both the difficulty of debugging and the amount of time required for testing will increase exponentially with every new script added.
As @meredoth has pointed out you’re making the challenge of game creation especially difficult. In addition to regular problems you’ve added “what does my code do here” into the mix. Remember, “plain code is good code”.
I think it is great that you are writing a game to hone your skills but keep in mind that this game should provide at least 50% of the basics that your next game will need. Apps are made up of reusable parts.
Pay particular attention to “code smell”. When you see yourself writing things like the following snippet ask yourself “is there no better way”? In most cases there are and if you don’t know it at the moment it will come to you eventually so plan on fixing it soon.
Surely damage amounts can be determined without a ton of if/else conditional tests. Would this be your go to solution if there were 100 monster types?
If by Character Selection you mean changing the player’s visuals or behaviour or both, you can make your life a lot easier by regarding the “player” as different from the “character” (often called Avatar, not to be mistaken by the movie).
Use this hierarchical layout of the Player character where each level is a GameObject of its own:
Player (has scripts like Input, Motion, Stats, Animator, CapsuleCollider, etc)
Character
Mesh (using SkinnedMeshRenderer)
Root (bone hierarchy, has many children)
Changing the Character then becomes dead simple: Destroy the “Character” child object, instantiate a new one, and replace any references to the existing one. Or you could have several named characters already as children in the prefab, in that case you’d only SetActive(false) all of them and afterwards SetActive(true) the one you selected.
However, there’s one downside: each instance of Player now has each character and carries them along with it. It works without issues for a few player instances and a few characters, but if either scales up (ie Player is actually an enemy and there could be dozens) then that setup would be wasteful in memory and CPU cycles.
If all characters use the same bones and animations, you can also move Root as direct child of Player and just change the SkinnedMeshRenderer’s mesh and material to alter the visuals of the Player.
Destroying the Player instance itself is what gets everyone in trouble as now you’d have to somehow, somewhere store its position, its current state (including physics), and so forth. Therefore keep that root instance and only modify children of it.
Thank you for the feedback! You are right in that my school program focused more on certain skills/fundamentals over specific problems. Thank you for giving some insight on where to start! I was feeling a bit overwhelmed revisiting this project due to the size of it (as I was a bit too ambitious in what I was wanting to try to create and teach myself). I will likely provide some screenshots once I try out some of the solutions other people suggested.
Thank you for this advice! I have been in the process of debugging myself, but was struggling with resolving the mentioned issues, hence me reaching out here. I appreciate this resource and will use it in my continued debugging attempts!
Thank you so much for this in-depth response! I appreciate the time you took to identify these issues/discrepancies that may be contributing to this difficult debugging experience on my end.
The self variable was how my classes taught us to reference to the current instance object in the past. I wasn’t aware how redundant it was implementing this would overcomplicate things, so I will go back and edit these references to instead access the gameObject property directly.
I had implemented some of the character-selection logic into the ProtagMovement class when the dedicated script was not working as intended, and was an effort to apply this feature in a different way. I am in the process of trying to implement a more cohesive reference/connection to the CharaSelect script as you reccomended within the Start class.
The galPicked and guyPicked variables in the ProtagMovement class were intended to be references to which button was selected by the player in CharaSelect instance, as a way to verify the player’s choice was recognized. However, I did not realize how initializing them as false would disrupt this.
You are correct in that I currently have two player GameObject prefabs using this script, with one manually configured as the male character and the other as the female character through the Inspector. Then, based on the values in CharaSelect , the character which was not selected was destroyed. This was my attempt of implementing this feature through my introductory knowledge of how to script in Unity and manipulate objects in the Inspector. I am in the process of attempting to correctly instantiate the appropriate selected prefab as per your advice.
In addition to the camelCase convention I have applied, what would be a method you would recommend to better distinguish between fields, local variables, and other member types between accessibility levels? I would love to know how I can improve on this and create more readable and organized code.
Could you provide an example of that fields would be better changed from public to private or [SerializeField]? I want to better understand where I made these mistakes so that I can fix them accordingly (expecially [SerializeField] as we did not discuss that much in school).
Along the same lines, if a value only needs to be read from another script, expose it through a property with a public getter and no public setter. This makes it much easier to understand what data can be modified and from where. Right now, virtually everything can be changed from anywhere. I had to use GitHub’s “Find References” feature repeatedly just to determine whether a variable was being used and from where its value could change.
*I am in the process of my attmempts to properly cache the FindGameObjectWithTag and GetComponent references into Start instead of Update as well. I was not aware that doing this within Update would create further issues.
I am in the process of attempting to correct the error with the incorrect IEnumerable MagicShield(float damage) call now. It was originally made public because I was working on it at the same time as the potions and their effects (which are public due to being called in the Potions script) and so I had made it public mistakenly.
I truly appreciate your guidance and advice on where some of my major flaws to fix are, and will do my best to use this to better this project. I admit, I was so excited to try to make something new that I got a bit ahead of myself. I will review these fundamentals and try to apply your critiques to improve upon my current project.
Thank you for this insight! I realized as I revisited this project and resumed my debugging attempts that a good amount of my code is a bit more overcomplicated than I thought when initially writing it. I suppose that’s because I still have room to grow and learn. I will do my best to apply this advice as I continue pursuing this.
Does this still apply if each Player option is a custom prefab? Or would I have to create entirely new set of prefabs with this hierarchy for each option? I feel like in order to implement this I would need to completely rewrite the CharaSelect script itself, unless I am misunderstanding and there is an easier way to implement this fix without having to rehaul and start over from scratch.
It’s worth noting that when you call Destroy on a Unity GameObject, it is not destroyed immediately. Instead, it is destroyed at the end of the current frame.
Check the official Microsoft documentation on C# naming conventions:
One reason for following naming conventions is that they allow someone reading the code to quickly identify the type of member and, in many cases, its accessibility level without constantly scrolling back to its declaration. Understanding a variable’s accessibility is important because it tells you where and how its value can be modified.
If a variable needs to be read from and written to by another script, it should be public or, even better, exposed through a public property.
If another script only needs to read the value without modifying it, expose it through a property with only a getter.
If the variable does not need to be accessed from another script, make it private.
If it needs to remain private but should still be visible and editable in the Unity Inspector, use [SerializeField].
These are the basic access levels. For more information, see:
The main benefit is that when you see a private variable, you know it can only be modified by the class or struct in which it is declared. Nothing external can change its value. This makes code easier to understand and significantly simplifies debugging.
For example, if you have private int age = 100; and the value is never modified anywhere within the class, then you can be certain that an expression such as ageOfDeath = age + 10; will always produce 110.
If age were public, you could no longer make that assumption. You would first need to search through the project to find every script that can access age and determine whether any of them modify its value before that code executes.
I have some screenshots of the latest issues I am facing after working to integrate the debugging suggestions you all have given me.
This is the results I get when trying to create a V1 Build of the game. This is because when testing the Main Menu it stopped working and said it was because none of the game scenes were found in the build (I assume because this is the first time I’ve tested the game on my new PC).
The Main Menu and Game Screens work as intended once all game scenes were loaded into the Build Settings, thankfully, but this is what happens when the main game loop begins (for this I chose the Female Character Option)
It appears both player models spawn simultaneously outside of the labyrinth, and neither move or respond to motion controls. (There is also the new issue of the MiniMap objects appearing on the main camera).
Thank you for this clarification! Just pushed my current changes in beginning to apply your advice, and provided some screenshots into how things are looking now. I will definitely look into this and use them as references on how to repair some of my messy code. I truly appreciate it!
First error: the script the error mentions uses the PackageManager API which is editor-only. This needs to be removed or safeguarded with #if UNITY_EDITOR.
Btw please post logs (any text) as text. You can select one or more Console entries and hit Ctrl+C. That image is hard to read, and we can’t quote, edit, or search image contents.
If this simple hierarchy change forces a complete rewrite of a script, you may have written that script in an unmaintainable manner eg by hardcoding all assumptions. Moving something one level down or up in the hierarchy should be a straightforward change, ideally just the 1-3 lines that deal with creating, destroying, and getting the target object or its components.
