- Home /
Problem with elevator script (C#)
I'm trying to make a script that allows the player to open doors and chests and activating elevators, however I've bumped into a few problems, for some reason it can't find my player character. The other problem is that I've tried make the defined thingie for my player public and draged the player prefab there from the inspector while in-game and then tried activating the elevator to see if it works but nothing happends.
Also since I might use this script on quite a few of my objects in-game (for chests etc) I'm not sure if it's smart having it reading if the object is selected in every frame in case it needs to perform the interactive actions, does anyone know another way I could set the interactive part up?
using UnityEngine; using System.Collections;
public class Button : MonoBehaviour { public GameObject target; public bool allowReset = false;
public bool elevator = false;
public Transform elevPointA;
public Transform elevPointB;
public bool door = false;
public bool loot = false;
public int maxDistance;
public int Speed;
private bool _selected = false; //is the player marking this object with the mouse
private Transform _player;
private Transform _myPosition;
private Transform _targetTransform;
/// <summary>
/// State 0 = Elevator PointA, Door closed, Loot closed
/// State 1 = Elevator PointB, Door open, Loot looted
/// State 2 = Elevator moving
/// </summary>
private int _state = 0;
void Awake() {
_targetTransform = target.transform;
_myPosition = transform; //this is this objects transform
elevPointA = transform;
elevPointB = transform;
}
void Start() {
GameObject fp = GameObject.FindGameObjectWithTag("Player"); //find the gameObject tagged Player
_player = fp.transform; //read gameObject Player as a transform
}
void OnMouseEnter() {
_selected = true;
renderer.material.SetColor ("_OutlineColor", Color.blue);
}
void OnMouseExit() {
_selected = false;
renderer.material.SetColor ("_OutlineColor", Color.black);
}
void Update() {
if(Input.GetMouseButtonUp(0)) {
if(_selected == true) {
if(Vector3.Distance(_player.position, _myPosition.position) > maxDistance) {
_state = 1;
if(elevator == true)
Elevator();
else if(door == true)
Door();
else if(loot == true)
Loot();
}
}
}
// stop the elevator and reset the button if the elevator has reached CheckPoint A if(Vector3.Distance(_targetTransform.position, elevPointA.position) == 0) { if(_state == 0) { return; } else { Speed = 0; _state = 0; } } // stop the elevator and reset the button if the elevator has reached CheckPoint B if(Vector3.Distance(_targetTransform.position, elevPointB.position) == 0) { if(_state == 1) { return; } else { Speed = 0; _state = 0; } } }
void Elevator() {
if(_state == 0) {
_targetTransform.Translate(Vector3.up * Speed * Time.deltaTime);
_state = 2;
}
else if(_state == 1) {
_targetTransform.Translate(Vector3.up * Speed *Time.deltaTime);
_state = 2;
}
}
void Door() {
}
void Loot() {
}
}
(I'm usually just doing graphics but have begun coding just recently so please excuse me if my script is a mess.)
Answer by skovacs1 · Nov 03, 2010 at 04:37 PM
The code is actually nicely formatted. The logic is very broken though.
In your Awake, you assign various transforms. None of them are needed and the very presence of some of those assignments breaks your functionality:
- Why are you assigning both
elevPointA
andelevPointB
to be the currenttransform
? by doing this, you are storing 3 separate references to the current transform and when you write code likeVector3.Distance(_targetTransform.position, elevPointA.position) == 0
and then laterVector3.Distance(_targetTransform.position, elevPointB.position) == 0
, both will be true and false at the same time because they are the same transform. - If you are only ever using the
Transform
oftarget
, then why not just make thatpublic
and drop theGameObject
altogether. transform
ispublic
, you don't need to store it for what you're doing with it
In your Elevator(), you only ever go up. Is this intentional? Also, speed is only ever set to 0 here. Because you are moving up by some floating point value which you can't be sure divides evenly into the distance between target's transform.position and elvePointA.position or elevPointB.position, it is likely that the distance between then will never be 0 and your if statements (which essentially both do the same thing and should be combined) will never be true.
Your functions and variables are poorly named and this makes the code harder to understand. What does it mean for elevator to be true? Does it mean that the button controls an elevator? If it can only control one type of thing at a time, then you should only need to store one state variable to represent this. You should consider naming your functions verbs or events and your variables what they actually mean (like controlsDoor or something that can actually be said to be true or false, unlike simply door which does not indicate something that is true or false). For complex state variables, you might consider using enumerators to improve readability.
If you're worried about efficiency, you should rearrange your logical comparisons to do the cheaper operations first and to do computation less often by merging your statements. Also, using squared distance is cheaper than distance because it requires one less square root and square roots are fairly expensive. You should also combine your logical comparisons into the same clause if appropriate using AND (&&) and OR (||), because while it's not all that expensive, it's a matter of readability and ease of following the logic.
Should a button really store what it is a button for? Should it do all the controls? If that's the case then you should have specialized buttons for each thing, rather than going for a generalized button. If you want a generic button, it's better to put that logic with the object being controlled so that if you wanted your buttons to control other things, then all you'd have to do is write the appropriate script for your other thing and you wouldn't have to worry about the button.
The following is simply an example of how to break up the code:
Button.cs (Attached to buttons)
using UnityEngine;
public class Button : MonoBehaviour { public GameObject target; public float maxSqrDistance = 900.0f; public bool pushed = false;
private Transform player;
void Start() {
player = GameObject.FindGameObjectWithTag("Player").transform;
}
void OnMouseEnter() {
renderer.material.SetColor("_OutlineColor", Color.blue);
}
void OnMouseExit() {
renderer.material.SetColor("_OutlineColor", Color.black);
}
void OnMouseUp() {
if((player.position-transform.position).sqrMagnitude < maxSqrDistance) {
pushed = true;
target.SendMessage("ButtonPushed", transform);
}
}
void Done() {
pushed = false;
}
}
Elevator.cs (Attached to elevators)
using UnityEngine; using System.Collections.Generic;
public class Elevator : MonoBehaviour { public Transform[] targetPositions; public float minTransitionTime = 2.0f; public float maxSpeed = 5.0f; public float timeToWait = 3.0f; private Queue<Transform> buttonRequests = new Queue<Transform>(); private KeyValuePair<Transform,Transform> current = new KeyValuePair<Transform,Transform>(null,null); private float yVelocity = 0.0f; private float waitTime = 0.0f; private bool waiting = true;
void ButtonPushed(Transform button) { //Queue up the request
if(!buttonRequests.Contains(button)) buttonRequests.Enqueue(button);
}
void Update() {
if(targetPositions.Length == 0) return;
if(waitTime > 0.0f) waitTime -= Time.deltaTime;
if(waiting && waitTime <= 0.0f) { //Done waiting
//Clear destination
if(current.Key != null) {
//Do stuff here like sending the elevator to another position
//This depends on your elevator setup
current = new KeyValuePair<Transform,Transform>(null,null);
}
//Get a new destination
if(buttonRequests.Count == 0) return;
Transform nextButton = buttonRequests.Dequeue();
//Find the closest position to the button
Transform nextPosition = transform;
float minDistance = Mathf.Infinity;
float currentDistance = 0.0f;
foreach(Transform targetPosition in targetPositions) {
currentDistance = Mathf.Abs(targetPosition.position.y
-nextButton.position.y);
if(currentDistance < minDistance) {
minDistance = currentDistance;
nextPosition = targetPosition;
}
}
//Set to move to the position nearest the button
current = new KeyValuePair<Transform,Transform>(nextPosition,
nextButton);
waiting = false;
}
if(!waiting) {
//Move towards our destination
float newPosition = Mathf.SmoothDamp(transform.position.y,
current.Key.position.y,
ref yVelocity,
minTransitionTime,
maxSpeed);
transform.position = new Vector3(transform.position.x,
newPosition,
transform.position.z);
//We've reached our destination
if(Mathf.Abs(transform.position.y-current.Key.position.y)<0.03f) {
//Do stuff here like opening doors, etc.
waiting = true;
waitTime = timeToWait;
current.Value.gameObject.SendMessage("Done");
}
}
}
}
Lootable.cs (Attached to lootable objects)
using UnityEngine;
public class Lootable: MonoBehaviour { void ButtonPushed(Transform button) { //Whatever you do when you loot something } }
Door.cs (Attached to doors)
using UnityEngine;
public class Door: MonoBehaviour { void ButtonPushed(Transform button) { //Open/Close the door } }
Hmm, the thing about the elevator only beeing able to go up is totally me beeing sloppy unfortunately, sorry for that. The fact why I want my player to be stored in private is 'cause my game loads data from saved files and generates the player and spawns him at a spawn point so my player is never in the inspector.
Thanks alot for your help, you gave me alot to think about! I'll go through your script tomorrow and check out how you've done it. And thanks again, I learned alot from this. :)
I never said anything about player being public, did I? I said in your original script that your target Transform (which seemed in this case to be the target the button controls) should be a public variable and set in the inspector so you wouldn't have to look it up like you were-(I only used the GameObject, so I didn't need this change in my code).
Oh, sorry I must've misread and confused myself. I've tried using your script however the elevator still doesn't move, have looked through the code but I don't quite understand where the is, I tried debuging. I want to check if the script has found any targets to move toward when moving but I get errors like "cannot convert type float to Unityengine.Object" and such. I remember I read about something like this a while back and you could typecast or bypass this somehow but can't find where I read it, how did I do this? :P
The reason it wasn't moving was because of the way waitTime was being used and checked as well as current. Another correction needed was using the absolute value for calculating the closest position. Also, another problem I found that I didn't know about was that SmoothDamp never reaches the target exactly, but gets very close. I have corrected the script.
I'm not sure where you got your type mismatch from because I didn't run into those when I was cleaning this up. To typecast, you can do something like '(Type)expression' or the safer way is to use the as keyword 'expression as Type'.