The rules are simple - review the snippet of code in the post above yours, and then paste a snippet of your own code for review by your fellow programmers of the Unity forums. This is a fun little exercise and a great way to learn crazy wicked cool new ways of writing code and solving problems.
Rules: 1. Use code tags!
public float thisIsHowWeRoll = Mathf.Infinity;
2. Post what your code is supposed to do
It’s hard to verify that it’s functional if people don’t know what the function is supposed to be
3. Keep feedback constructive
Everyone knows that curly braces should start on a newline, but you’re not a terrible programmer if you choose to keep them on the same line (just a terrible person ). Post feedback related to logic flow, not stylistic choices.
4. Post Code to Review Code
Make sure and post a snippet of your own code for review when you review someone else’s - CODEBUSTERS is a non-spectator sport.
I’ll kick it off. This is a terrain script ripped out of an infinite sidescroller we’re working on:
using UnityEngine;
using System;
using System.Collections;
using System.Collections.Generic;
using System.Linq;
//We assume that terrain blocks pivot at the left side, FYI
public class Terrain : MonoBehaviour {
public TruckMovement truckMovement;
public Transform[] terrainBlocks;
public float screenEdgePadding = 20; //how much we compensate for the perspective
public float terrainBlockWidth = 40;
float _currentTerrainX; //this is the point at which we will spawn the next terrain block
void Start(){
for (var i = 0; i < terrainBlocks.Length; i++)
terrainBlocks[i].position = new Vector3 (i * terrainBlockWidth, terrainBlocks[i].position.y, terrainBlocks[i].position.z);
InitialSetup();
}
[ContextMenu("Execute Initial")]
void InitialSetup(){
foreach(var curBlock in terrainBlocks)
curBlock.transform.position = Vector3.right * (TerrainLeft - terrainBlockWidth - 1); //shove all the blocks off screen
_currentTerrainX = TerrainLeft;
while(NeedsBlockInFront()){
PlaceNextBlock(NextTerrainBlock());
}
}
void Update(){
while(NeedsBlockInFront()){
PlaceNextBlock(NextTerrainBlock());
}
}
public void PlaceNextBlock(Transform block){
block.transform.localPosition = Vector3.right * _currentTerrainX;
_currentTerrainX += terrainBlockWidth; //TODO: make this support arbitrary block lengths
}
/// <summary>
/// Determines if we need to catch up
/// </summary>
/// <returns><c>true</c>, if block in front was needsed, <c>false</c> otherwise.</returns>
public bool NeedsBlockInFront(){
//TODO: compensate for blocks being visible past the gameplay edge (in distance perspective)
return _currentTerrainX < TerrainRight;
}
/// <summary>
/// Determines if a block has been left too far behind to be seen anymore
/// </summary>
public bool BlockHasFallenOff(Transform block){
//TODO: we need to handle different sized blocks
return (block.transform.position.x < TerrainLeft);
}
public float TerrainLeft{
get {
return GameScreen.Left - terrainBlockWidth - screenEdgePadding;
}
}
public float TerrainRight{
get{
return GameScreen.Right + screenEdgePadding;
}
}
/// <summary>
/// Calculates which block should go next in the generated terrain
/// </summary>
/// <returns>The terrain block.</returns>
public Transform NextTerrainBlock(){
//TODO: handle different terrain styles
//TODO: get rid of linq
return terrainBlocks.First(x => BlockHasFallenOff(x)); //TODO: IF A GAMEOBJECT IS INACTIVE, It can be used
}
}
The purpose of this code is to take a list of GameObjects of equal length and lay them out in a horizontal pattern. When the camera moves past the right edge of the list of objects, we pull one off the left edge and place it in front.
For the most part looks good, maybe a little convoluted in places: on Start() you move all the blocks, then you call InitialPosition which moves all the blocks then you call place next block which moves some of the blocks.
You also might want to consider moving some of the functions to a block class, although it would only be worthwhile if things are likely to get more complex.
Suppose I best post something to keep in the spirit of initial post.
var damageLevels:int = 0;
var damageForce:float = 5.0;
var emitter:ParticleEmitter;
var textureOffset:float = 0.25f;
private var currentDamage:int;
private var mass:float;
static var BOUNCE_FORCE : float = -0.33;
function OnCollisionEnter (collision:Collision) {
if (GetAppliedForce(collision, rigidbody) >= damageForce) {
DoDamage(GetAppliedForce(collision, rigidbody), collision.rigidbody);
}
}
function DoDamage(f:float, r:Rigidbody) {
if ((currentDamage + Mathf.Floor(f / damageForce)) < (damageLevels + 1)) {
renderer.material.mainTextureOffset.y -= textureOffset * Mathf.Floor(f / damageForce);
currentDamage += Mathf.Floor(f / damageForce);
} else {
if (emitter != null) emitter.emit = true;
if (r != null) r.AddForce(r.velocity * BOUNCE_FORCE, ForceMode.VelocityChange);
gameObject.active = false;
}
}
function GetAppliedForce(c:Collision, r:Rigidbody):float {
if (c.rigidbody != null) return ((r.velocity * r.mass) - (c.rigidbody.velocity * c.rigidbody.mass)).magnitude;
return (r.velocity * r.mass).magnitude;
}
Handles cycling damage texture + particles like this:
This is pretty old, haven’t been using UnityScript much lately, although this kind of makes me want to do so
You’ve got some inefficient recalculation of stuff, e.g. on lines 13 and 14 you’re called GetAppliedForce twice with the same arguments and the result won’t have changed so you could calculate it once into a temporary variable which would be cleaner and shorter. Similarly in DoDamage you’re repeating calls to Floor.
‘currentDamage’ would be better named as ‘currentDamageLevel’ as it makes the relationship between it and ‘damageLevels’ clearer. I’d probably rename the latter to ‘maxDamageLevel’ or ‘numDamageLevels’ though. As it is, ‘currentDamage’ makes me think it could be a float value that stores the damage fractionally between levels. Also ‘damageForce’ is a bad name; it should be ‘minForceMagnitude’ or something
Depending on how often this code is run, you’re using shortcut properties for Rigidbody and Renderer; these are equivalent to GetComponent calls, so if you’re getting a lot of collisions then you might be better off retrieving the components in Awake() and storing them for later use. (In Unity 5, these shortcut properties are going to be removed and your code upgraded to actually be GetComponent calls, fwiw).
On line 25 you should really be using SetActive() rather than .active, which I believe is obsolete.
OK, my turn to go dig up something. Hrrrmmm…
Here’s a component from some 2D platformer stuff I was messing around with. It’s for generating a ‘rope bridge’ kinda thing out of individual segments connected by hinge joints. You specify how many segments you want in the bridge, and the position of the end of the bridge in local space (so e.g. if you specify 6 segments but an offset of only (5, 0) then it’ll sag down a bit) and it sets everything up for you.
using UnityEngine;
using System.Collections;
public class Flexibridge : MonoBehaviour {
public int NumSegments;
public Vector2 ToOffset;
public Rigidbody2D SegmentPrefab;
void OnDrawGizmos()
{
if(Application.isPlaying) return;
Gizmos.DrawLine (transform.position, transform.position + (Vector3)ToOffset);
}
void GenerateBridge()
{
Vector2 localAnchor, otherAnchor;
if(ToOffset.x > 0)
{
localAnchor = new Vector2(0, 0);
otherAnchor = new Vector2(1, 0);
}
else
{
localAnchor = new Vector2(1, 0);
otherAnchor = new Vector2(0, 0);
}
Rigidbody2D[] segments = new Rigidbody2D[NumSegments];
HingeJoint2D[] joints = new HingeJoint2D[NumSegments];
for(int i = 0; i < segments.Length; ++i)
{
segments[i] = (Rigidbody2D)Instantiate (SegmentPrefab);
segments[i].transform.parent = transform;
segments[i].name = i.ToString();
joints[i] = segments[i].gameObject.AddComponent<HingeJoint2D>();
joints[i].connectedBody = (i > 0) ? segments[i - 1] : null;
joints[i].anchor = localAnchor;
joints[i].connectedAnchor = (i > 0) ? (otherAnchor) : (Vector2)transform.position;
joints[i].transform.position = transform.position + new Vector3(Mathf.Sign (ToOffset.x), 0, 0) * (i);
}
var endHinge = segments[segments.Length - 1].gameObject.AddComponent<HingeJoint2D>();
endHinge.anchor = otherAnchor;
endHinge.connectedAnchor = (Vector2)transform.position + ToOffset;
}
public void Start() { GenerateBridge(); }
}
Your variable names are much nicer, but this was Unity 3 so I’m not sure SetActive was available back then. Multiple calls could certainly be wrapped in temp variables, this is one thing I fail to do quite often even when writing “serious code”. I’m not too concerned about the performance in this case, but it would likely make the code easier to understand.
I’ll leave it to someone else to analyse your code, I don’t want to close the loop.
This is a piece I have written almost a year ago. I always use to make a health system that darkens your screen with some blood texture
using UnityEngine;
using System.Collections;
public class PlayerHealth : MonoBehaviour
{
public float currentHealth; //The current amount of health we have
public float maxHealth; //The maximum amount of health to have
public float healthRegenTime; //Time before health starts to regenerate
public float healthRegenAmount; //The amount of health to regenerate per second
public Texture2D dmgTexture; //The texture that will act as the damage indicator
private float untilRegen; //The time until we can start regenerating health
private Rect rectZero; //Used to hold the position and scale of the damage texure
void Start()
{
//Make sure the player starts at full health
currentHealth = maxHealth;
//Make the Rect in the zero position, with the height and width the same as the screen
rectZero = new Rect(0, 0, Screen.width, Screen.height);
}
void Update()
{
//Regenerate health
HealthRegen();
}
void OnGUI()
{
Color newColor = GUI.color; //Used to hold the new color value of the texture
newColor.a = GetHealthPercentage(); //Set the new alpha to the texture according to the current health
//Set the color of the texture to the updated one
GUI.color = newColor;
//Draw the texture on the screen so that it fits the screen completely
GUI.DrawTexture(rectZero, dmgTexture, ScaleMode.StretchToFill);
}
void HealthRegen()
{
//If the current health is less than maximum health and the regen timer is over...
if (currentHealth < maxHealth && Time.time > untilRegen)
{
//Add 1 health to the current health every second
currentHealth += healthRegenAmount * Time.deltaTime;
//Make sure the current health doesnt exceed the maximum health
if (currentHealth > maxHealth)
currentHealth = maxHealth;
}
}
void ApplyDamage(float damageToApply)
{
//Subtract the damage amount to the current health
currentHealth -= damageToApply;
//If we have less than 0 health then end the game
if (currentHealth <= 0f)
return; //end Game ==============================================================================================
//Start the health regen timer
untilRegen = Time.time + healthRegenTime;
}
float GetHealthPercentage()
{
//Calculate the percentage of health left and return the value
float newValue = currentHealth / maxHealth;
newValue = 1 - newValue;
return newValue;
}
}
Edit: This is code that I have written solely by myself. If you perhaps would like to use this code, please feel free to do so as I see a lot of people asking how to do blood health screens. Thanks!
From a technical perspective, this looks pretty well done - nice clean code and does what it says on the tin (bonus points for gizmos!). Of note: when I ran it, all of the rigidbodies were asleep on Start, so the bridge didn’t fall - I assume we’ve got different physics settings.
From a functional perspective, I’d prefer something with a bit more flexibility in terms of control - mostly a way of defining how far apart the bridge pieces are (without that, there’s not really a good way to control the sag).
Not really a point in this case - accessing transform is not terribly slow, and the bridge generation is only run once.
Nice!
You seem to have a pretty good grasp of the language, which is good. I’d recommend thinking through your program structure - it sends up red flags when you have a single class controlling a GUI item(blood screen), player health, and win condition all at the same time. If you were to rewrite it, I’d recommend splitting it up into:
A health script that handles taking damage and regeneration
A blood screen script that fades your texture in and out based on a health script (it could be the player’s health, or even an NPC’s health).
A game script that handles the win condition - i.e ends the game when a certain Health’s currentDamage drops below zero.
So this script is from years ago (back when I used US… I kind of miss it in a sick way) designed to cycle through a sprite atlas to produce a sprite animation.
#pragma strict
var atlas : Texture2D;
var uvs : Rect[];
var fps = 24;
var loop = false;
var playOnStart = true;
var isPlaying = false;
private var _currentFrame = 0;
function Start()
{
if(playOnStart) Play();
}
function Play() : IEnumerator
{
renderer.enabled = true;
isPlaying = true;
_currentFrame = 0;
while(_currentFrame < uvs.length)
{
var curUV = uvs[_currentFrame];
renderer.material.mainTexture = atlas;
renderer.material.mainTextureOffset = Vector2(curUV.x, curUV.y);
renderer.material.mainTextureScale = Vector2(curUV.width, curUV.height);
yield WaitForSeconds(1.0 / (fps + 0.0));
_currentFrame ++;
}
if(loop) Play();
else Stop();
}
function Stop()
{
isPlaying = false;
StopAllCoroutines();
renderer.enabled = false;
}
Thanks. I didn’t know people usually split scripts up that much. Is that what AAA studios usually do? Thanks for the suggestions too.
Also, I also noticed that in your code, you are not changing the var loop at all within the script. I assume you are controlling it in another script? It’s a good thing we got U4.2 so we don’t have to worry about sprites anymore huh haha
I just wanted to compliment you for the neatness of your code. It’s people like you that makes life easier
One thing that annoyed me was that you lacked comments… maybe you just took them out or something but keep comments gets rid of a lot of speculation / questions. Anyways, code looks nice. One question, this code is only run once. That means that you are not going to use those bridge pieces again in another script? Because I don’t see a reference to some sort of pooling system or anything. Possibly there is no level scrolling or there is only one bridge at the beginning of the level?
When using component oriented design, yes, it’s generally considered good practice to keep each component to having one role or responsibility. Think about, for instance, if you later decided that you wanted stuff other than health to control the screen image?
Also, many or verbose comments aren’t always a good thing. There’s a school of thought that you should have a minimum number of comments in favor of “self documenting code”, with the argument being that comments themselves need maintaining and can be ‘buggy’, and an incorrect comment can cause far more damage than not having a comment. Comments should generally explain why you’re doing things, not what you’re doing. In a quick skim of that code there’s nothing that I don’t think I grasped immediately, so comments could be an overhead without a benefit.
Not in this case, but in general it will make your code slightly faster, and if we’re picking apart code here, may as well throw all the optimisations in
@flaminghairball I wouldn’t consider myself anything more than a novice when it comes to programming so please take this with a grain or three of salt. But when reading over your animation snippet I didn’t see anything that jumped out as needing refactored.
The only thing that really caught my eye, is the disabling of the renderer as a whole when the animation was complete. I would feel safe to assume that this is hooked into a controller which would probably choose the next animation set after this was finished, but couldn’t this cause a frame or two where nothing is rendered?
@superpig A bit out of order, but has been eating at me with your snippet and I was just curious as to your decision to due so - On line 50, you dynamically type for the HingeJoint2D when you’ve been static typing the rest of the class. Was there any conscious reason behind this? Again, I am just curious.
Had to do a bit of digging to find something that didn’t have it’s teeth hooked into several other components, so this is from a fairly dated project and is probably due for its share of refactoring.
Thanks! Yes, at the moment the script assumes a piece width of 1 unit, which should be made controllable (or should at least be calculated from the prefab). The existing controls might have been OK if the gizmo was actually drawing what the at-rest bridge shape would really be, rather than just a straight line from A to B. As it is I was placing the component, setting the end offset, and then just tweaking the number of segments + popping into play mode to look at the result, and iterating that way - not the fastest thing in the world on a larger project.
As angrypenguin said, comments aren’t always a good thing - I actually try to avoid them wherever possible because I aim to make the code always be clear enough through identifier names and structure. I think my snippet isn’t great in this regard; names like ‘otherAnchor’ are not really good, and while it’s easy enough to read what lines 21 through 31 are doing, it’s not sufficiently clear why they’re doing that. A comment would actually have been appropriate in that case.
Yeah this wasn’t designed to spawn/unspawn the bridge as the player moves through the level - it’s really just a utility to avoid me placing the pieces into the scene file by hand. I’d have multiple bridges but they’d just instantiate multiple segments. If I were making the game for mobile, this would certainly be a thing worth considering, though I’d want to profile the cost of the sleeping bridges first.
Actually, there’s been a change in a recent build - I think a 4.X build, but it might have been a 5.X - that transform is now cached on the Mono side of things. As such there’s almost zero performance overhead (possibly actually zero, if the compiler is smart) of accessing .transform multiple times, compared to caching it in a local variable. You’re right about transform.position though - it would be slightly faster to calculate it just once, instead of repeatedly hopping the managed->native boundary and walking the transform hierarchy. Still, yes, not a super-high priority for run-once-on-startup code
Ah, good catch! No, I don’t think there’s any conscious reason for that - usually I use ‘var’ much more than I did in that snippet so I’m not entirely sure why I only used it for line 50 and not the rest. It’s not great that I’ve been inconsistent.
As a general style thing, I’d consider using both “m_” prefixes and “this.” member accesses to be redundant, especially when in code like this you’re not really accessing any other objects. So the code is a bit more verbose than it could be I think.
I don’t see a definition for AffectedRigidbodies (line 40) anywhere? And I can’t see where m_EffectedBodies is actually read from. Were both names intended to refer to the same thing? If so I’d probably make it a HashSet rather than a List; you lose the ability to see it in the debug Inspector, but your Contains() check on line 40 would be faster.
You didn’t say what the code is actually designed to do, but I think I follow it more or less. As an overall approach I think I might set things up using a separate sphere collider (with radius == this.m_Range) and trigger messages rather than FixedUpdate - it should be more efficient than doing OverlapSphere multiple times every frame - and I’d consider getting all the objects with rigidbodies onto their own layer, so you can set the physics layers up to filter the collisions appropriately instead of having tests like lines 34-37.
Beyond these, you’re also accessing this.rigidbody multiple times when it could be worth caching it; and I’m not a fan of doing ‘== true’ comparisons when the code reads naturally without it (i.e. “if RB is this rigidbody, or affected bodies contains RB, then” reads naturally already, while “or affected bodies contains RB equals true” is less natural).
Alright, here’s another one from me. The following code defines a ‘Skeleton’ component, which I place on the root bone in a character’s skeleton hierarchy. I use it for doing stuff like swapping out character skeletons (e.g. swapping between a regular animated transform hierarchy and a ragdoll hierarchy) as well as quickly finding particular bones in the skeleton and managing the activation state as I pool/unpool skeletons.
using System;
using System.Collections.Generic;
using System.Linq;
using UnityEngine;
public class Skeleton : MonoBehaviour, ISkeleton
{
public List<Transform> Bones;
public List<Transform> UnindexedTransforms;
[ContextMenu("Capture bones")]
public void CaptureBones()
{
var bones = GetComponentsInChildren<Transform>(true).ToList();
bones.Sort((a, b) => Comparer<string>.Default.Compare(transform.MakeRelativePath(a), transform.MakeRelativePath(b)));
Bones = bones.ToList();
}
public void CopyPoseTo(Skeleton target)
{
CopyPoseTo(target, true);
}
public void CopyPoseTo(Skeleton target, bool scaleRootOnly)
{
Assert.Equal(Bones.Count, target.Bones.Count);
// Assume that the target skeleton has the same bone sort order as us);
for (int i = 0; i < Bones.Count; ++i)
{
target.Bones[i].localPosition = Bones[i].localPosition;
target.Bones[i].localRotation = Bones[i].localRotation;
if(!scaleRootOnly)
target.Bones[i].localScale = Bones[i].localScale;
}
if (scaleRootOnly)
target[target._rootName].localScale = this[_rootName].localScale;
}
private Dictionary<string, Transform> _boneLookup;
private string _rootName;
[ContextMenu("Rebuild lookup")]
public void RebuildBoneNamesLookup()
{
if(Bones == null)
throw new InvalidOperationException("No bones set");
int token;
VerifyingProfiler.BeginSample("Rebuild bone names lookup", out token);
if(_boneLookup == null)
_boneLookup = new Dictionary<string, Transform>(Bones.Count);
_boneLookup.Clear();
for (int i = Bones.Count - 1; i >= 0; --i)
{
if (!Bones[i])
{
Bones.RemoveAt(i);
continue;
}
_boneLookup.Add(Bones[i].name, Bones[i]);
}
_rootName = transform.name;
VerifyingProfiler.EndSample(token);
}
public void RenameRoot(string newName)
{
name = newName;
_boneLookup.Remove(_rootName);
_boneLookup.Add(newName, transform);
_rootName = newName;
}
public virtual void Awake()
{
UnindexedTransforms = GetComponentsInChildren<Transform>().Except(Bones).ToList();
RebuildBoneNamesLookup();
}
public void OnEnable()
{
if(_boneLookup == null || _boneLookup.Count != Bones.Count) RebuildBoneNamesLookup();
}
public virtual void OnSpawned()
{
for (int i = 0; i < Bones.Count; ++i)
Bones[i].gameObject.active = true;
for (int i = 0; i < UnindexedTransforms.Count; ++i)
UnindexedTransforms[i].gameObject.active = true;
}
public virtual void OnDespawned()
{
for (int i = 0; i < Bones.Count; ++i)
Bones[i].gameObject.active = false;
for (int i = 0; i < UnindexedTransforms.Count; ++i)
UnindexedTransforms[i].gameObject.active = false;
}
public Transform this[string boneName] { get
{
if (boneName == null) throw new ArgumentNullException("boneName");
if (_boneLookup == null) { RebuildBoneNamesLookup(); }
Transform result;
if (_boneLookup.TryGetValue(boneName, out result)) return result;
throw new KeyNotFoundException("Bone '" + boneName + "' could not be found in the skeleton.");
} }
T ISkeleton.GetComponent<T>()
{
return GetComponent<T>();
}
}
Criticising my own code quickly: I’m using .active in OnSpawned/OnDespawned when I should be using SetActive; and my calls to RebuildBoneNamesLookup() feels like it comes from me not having a solid grasp on when I actually need to call it and instead am just peppering checks/calls all over the place ‘just in case.’ But what else you got?
What are the benefits you find from using var over a static typing? I try to avoid using it as I think it increases readability to see the typing up front, but that’s only my preference.
Ah yes, this was an awkward time in my life where I was torn between trying to carry over some prefixes from my short lived C++ life and couldn’t decide between the this. prefix and the ugly notations I used when toying with Irrlicht and other rendering libraries some years ago. Trust me, I had some counseling and this has been resolved
I do apologize for the slip on line 40, I pulled this from an old SVN repo and assumed that since it was pushed it must have been valid; not halfway between refactoring. The line should read as m_EffectedBodies. I haven’t looked into the HashSet before, and to be completely honest haven’t even heard of it - though now I feel I should.
These are excellent points indeed, and I have seen other projects and assets do things as described here. If I ever revisit this prototype I will definitely be taking this into consideration. Reading back over the class, I can agree about the natural readability of * == true comparisons. I am sure at the time I did the comparisons to signal what the return value of the method would be, but on the other hand it is made obvious by the if statement.
I apologize that I can’t be of more assistance regarding your latest code posting, but aside from the SetActive which you’ve already acknowledged, it reads as a clean piece of code to me.
If no one is opposed, here is another from me. The class is the controller for a bomb object within a networked prototype, whose purpose is to handle creation and destruction of the bomb object while syncing rigidbodies and distributing messages to the clients of who was damaged in the process.
Having never worked in a team with another programmer, I really appreciate what and where this thread is going with allowing peer review of each others code. I really hope this thread goes the distance!
). Post feedback related to logic flow, not stylistic choices.
