Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Kim Ye Seung, 21.09.30 #8

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 57 additions & 0 deletions KimYeSeung/210930/Scripts/BGM.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
using System.Collections;
using System.Collections.Generic;
using UnityEngine;
using UnityEngine.SceneManagement;

public class BGM : MonoBehaviour
{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이름을 보고 뭘 하는 스크립트인지 이해하기 어렵습니다.
설명해 주신 대로 BGM을 관리하는 스크립트라면 BGMManager와 같이 스크립트에서 다루는 것과 작업이 나오도록 명명해주시면 좋을 것 같네요.

public AudioClip BGM1;
public AudioClip BGM2;
public AudioClip BGM3;
AudioSource audioSource;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이 경우에 BGM이 여러개 생기면 변수를 그만큼 생성해야겠죠? 이런 걸 하드코딩이라고 부릅니다.
그래서 AudioClip 자료형의 배열을 생성해 BGM을 여러개 만드는 것이 확장성이 좋고 편합니다.

public AudioClip[] BGMClips = new AudioClip[3];

...
audioSource.clip = BGMClips[0];

이렇게 사용하면 인덱스 일일이 만들 필요 없이 편하고 좋겠죠?

public playermove Player;
public GameObject player;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

playermove 클래스의 인스턴스인 Player, GameObject 클래스의 인스턴스인 player. 이 둘을 코드만 이름만 보고 차이를 확인할 수 있나요? 또, 이들을 public으로 두었을 때 다른 클래스에서 이를 보게 되면 어떻게 처리해야 할까요?
또한 player 필드가 Player 필드의 gameObject 필드라면 굳이 이렇게 나눌 필요가 있었을까요?


Rigidbody2D rigid;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • public은 public 끼리, private는 private끼리. 변수들도 비슷한 것끼리 놓아야 보는 사람이 편하고 좋습니다.
  • AudioSource audioSource에서 audioSource는 자료형 그대로 쓰고 Rigidbody2D rigid에서 rigid는 줄여 쓰는 데에 이유가 있나요? 둘 다 unity component이니까 통일해 주는 것을 추천합니다.
  • private도 생략하지 않는 것을 추천합니다.

void Awake() {
player = GameObject.Find("player");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
player = GameObject.Find("player");
player = Transform.FindWithTag("player").gameObject;

단순히 오브젝트의 이름만으로 player가 어떤 아이인지 찾아낼 수 있나요? Instantiate 메소드를 통해 생성된 오브젝트의 경우, 오브젝트 이름에 "(clone)" 이라는 추가적인 이름이 붙는다는 것 알고 계셨나요?
그러한 경우에는 플레이어를 어떻게 찾을 수 있을까요?

또, player는 public 필드라서 유니티 에디터 내에서도 할당해줄 수 있는 상태인데, 이미 맵에 생성되어 있는 player를 넣어주는 거라면 굳이 GameObject.Find 를 통해서 부하를 주는 것 보다 애시당초 에디터에서 넣어주는 방식이 훨씬 가볍지 않을까요?

this.audioSource=GetComponent<AudioSource>();
rigid = GetComponent<Rigidbody2D>();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this.audioSource에서 this는 audioSource로 생략할 수 있습니다. 바로 밑 줄에 rigid도 생략해 놨는데 왜 audioSource만 this가 붙어있죠?

}
void Start(){
Ms1();
}

void PlaySound(string action) {
switch(action){
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이건 위에서 말했듯이 배열로 바꾸면 필요 없겠지만 왜 int가 아닌 string으로 매개변수를 받는 건지 궁금합니다.
만약 배열을 쓰면

private void PlaySound(int index)
{
    audioSource.clip =  = BGMClips[0];
    audioSource.Play();
}

로 줄일 수 있겠네요.

case "0":
audioSource.clip = BGM1;
break;

case "1":
audioSource.clip = BGM2;
break;

case "2":
audioSource.clip = BGM3;
break;
}
audioSource.Play();

}

public void Ms1(){
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ms1() == PlayerSound("0");
Ms2() == PlayerSound("1");
Ms3() == PlayerSound("2");

완전히 같은 방식인데 굳이 MsXXX 라는 방식의 메소드로 랩핑할 필요가 있었나요? 오히려 가독성을 해치고 매개변수를 통해 유동적으로 처리할 수 있는 코드를 이렇게 동적이지 못하게 랩핑 할 필요가 있었나요?

보통 이런 경우를 '과한 기능' 혹은 불 필요한 랩핑이라고 많이 부릅니다.

PlaySound("0");
}

public void Ms2(){
PlaySound("1");
}

public void Ms3(){
PlaySound("2");
}

}

144 changes: 144 additions & 0 deletions KimYeSeung/210930/Scripts/Gamemanager.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,144 @@
using UnityEngine;
using System.Collections;
using UnityEngine.UI;
using UnityEngine.SceneManagement;


public class Gamemanager : MonoBehaviour
{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

C#에서 클래스 이름은 Pascal Case로 지어야 합니다. 변수 명명 형식에 대해서는 링크를 달겠습니다.
[Guide Name Casing]

public int totalPoint;
public int stagePoint;
public int stageIndex;
public int health;
public int HpPoint;

public playermove player;
public Image[] UIHp;
public Text UIPoint;
public GameObject UIRestartBtn;

public GameObject UIContinueBtn;
public GameObject UIEndBtn;
public GameObject Menu;
public GameObject Tutorial;
public GameObject MainStage;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

변수 이름을 지을 때 명명 스타일을 만들어서 지켰으면 합니다. 변수마다 이름 형식이 제각각이라 의미를 알아보기 어렵습니다.
다른 사람이 봤을 때 이 변수가 뭘 하는 건지 알 수 있을까 생각해 보면서 짰으면 좋겠습니다.


void Awake(){
health =3;
UIHp[3].color = new Color(0,0,0,0);
UIHp[4].color = new Color(0,0,0,0);
UIHp[5].color = new Color(0,0,0,0);
UIHp[6].color = new Color(0,0,0,0);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Color 클래스에 Color.clear 라는 (0,0,0,0)값의 정적변수가 있습니다.
UIHp[3].color = Color.clear 같이 쓸 수 있겠네요.
그리고 자주 사용하는 특정 상수 값이 있을 때는

private readonly emptyColor = new Color(0,0,0,0);
...
UIHp[6].color = emptyColor;

같이 변수로 명시해서 쓰는게 좋습니다. 변수를 애용합시다.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tyrosine1153 제안해주신 부분에 대해서, 상수로 쓰이는 readonly 의 경우는 static을 붙여주는 것이 좋습니다.
이는 모든 인스턴스에서 공통적으로 이용하는 상수의 경우 하나만 생성되어도 상관 없기 때문에 static을 붙이는 것입니다. 상수가 n개 생성되어도 문제가 있겠죠?

또. Color 구조체는 클래스가 아니기 때문에 const 처리가 가능합니다.

private static readonly EmptyColor = new Color(0,0,0,0);
or
private Const EmptyColor = new Color(0,0,0,0);
...
UIHp[6].color = EmptyColor;

health=HpPoint;
}

void Update() {
//점수 표시
UIPoint.text = (totalPoint + stagePoint).ToString();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
UIPoint.text = (totalPoint + stagePoint).ToString();
UIPoint.text = $"{totalPoint + stagePoint}";

보간 문자열을 아십니까? 위와 같이 처리할 수 있습니다.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ToString 메소드는 박싱을 일으키기 때문에 여기서 부하를 일으킬 수 있습니다. 박싱 언박싱에 대한 개념은 직접 구글링해서 찾아보고 꼭 이해하길 바라는 개념 중 하나입니다.


//메뉴
if(Input.GetButtonDown("Cancel"))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

여기서 쓰이고 있는 "Cancel" 의 경우, 유니티 내의 Input Manager에서 관리하고 있는 문자열과 각 버튼에 할당되어 있는 값입니다. 이는 코드를 읽는 사람이 보고 즉시 어떤 키를 눌러야 테스트가 가능한지 확인이 어려우므로 주석을 추가로 달거나, 아예 GetKeyDown(KeyCode.XXX) 를 통해 명시적으로 어떤 키를 눌러줘야 하는지 코드에서 알려주는 것이 좋습니다.

{
if(Menu.activeSelf)
{
Menu.SetActive(false);
}
else
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if문에 들어가는 Menu.activeSelf도 bool 값이니까

if(Menu.activeSelf)
{
    Menu.SetActive(false);
}
else
{
    Menu.SetActive(true);
}

같은건

Menu.SetActive(!Menu.activeSelf); 

같은 한줄로 정리할 수 있습니다.

{
Menu.SetActive(true);
}

}
}




public void NextStage(){
Tutorial.SetActive(false);
stageIndex++;
MainStage.SetActive(true);
PlayerRespawn();

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

vs code 코드 정렬 단축키는 ctrl + K, F입니다.
전체 정렬을 하려면 ctrl + A -> ctrl + K, F 로 하면 됩니다.


totalPoint += stagePoint;
stagePoint = 0;
}


//목숨 7개 아이템 먹음
public void UpHealth(){
HpPoint =7;
health=HpPoint;
UIHp[0].color = new Color(1,1,1,1);
UIHp[1].color = new Color(1,1,1,1);
UIHp[2].color = new Color(1,1,1,1);
UIHp[3].color = new Color(1,1,1,1);
UIHp[4].color = new Color(1,1,1,1);
UIHp[5].color = new Color(1,1,1,1);
UIHp[6].color = new Color(1,1,1,1);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

반복문을 사용합시다. 변수를 사용합시다.

for(int i = 0; i < 7; i++)
    UIHp[i].color = Color.Clear;


}

public void HealthDown(){
if(health > 1){
--health;
UIHp[health].color = new Color(1,0,0,0.4f);
}
else{
//플레이어 죽음
--health;
UIHp[health].color = new Color(1,0,0,0.4f);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

다른 if문과 중복되는 내용이 있다면 따로 빼주는 것도 좋습니다.

player.Ondie();
//Result UI
Debug.Log("UI 죽음 표시");
//Retry Button UI
UIRestartBtn.SetActive(true);
Text btnText = UIRestartBtn.GetComponentInChildren<Text>();
}
}

//HP 회복 아이템 먹음
public void HealthUp(){
health = HpPoint;
UIHp[0].color = new Color(1,1,1,1);
UIHp[1].color = new Color(1,1,1,1);
UIHp[2].color = new Color(1,1,1,1);
if(HpPoint==7){
UIHp[3].color = new Color(1,1,1,1);
UIHp[4].color = new Color(1,1,1,1);
UIHp[5].color = new Color(1,1,1,1);
UIHp[6].color = new Color(1,1,1,1);
}

}

public void Restart(){
SceneManager.LoadScene(0);
}

void OnTriggerEnter2D(Collider2D collision) {
//플레이어 떨어짐
if(collision.gameObject.tag == "Player"||collision.gameObject.tag =="playerdameged"){
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

태그마다 명명 형식이 다르네요. 통일하는 것이 좋습니다.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if(collision.gameObject.tag == "Player"||collision.gameObject.tag =="playerdameged"){
if(collision.CompareTag("Player") || collision.CompareTag("playerdameged")){

Collider2D 클래스에는 CompareTag() 라는 가비지 컬렉터를 호출하지 않고 태그를 비교해주는 메소드가 있습니다.
가비지 컬렉터에 대해서는 직접 구글링해서 찾아보고 꼭 이해하기 바랍니다. 중요합니다.

HealthDown();
if(health >= 1){
collision.attachedRigidbody.velocity = Vector2.zero;
collision.transform.position = new Vector3(-5,6,0);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

new Vector3(-5,6,0) 같이 자주 사용하는 상수는 readonly 변수로 만들어서 사용하는 것이 좋습니다.

}

}

//player Reposition

}
public void GameExit()
{
Application.Quit();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Application.Quit();
#if UNITY_EDITOR
UnityEditor.EditorApplication.isPlaying = false;
#else
Application.Quit();
#endif

Application.Quit() 메소드는 빌드를 한 후에만 먹히는 메소드로, 에디터 내에서는 실행되지 않습니다.
위와 같이 전처리문을 통해서 컴파일 되는 플랫폼에 따라 다르게 실행되도록 처리해주면 좋을겁니다.

}

void PlayerRespawn(){
player.transform.position = new Vector3(-5,6,0);
player.VeloccityZero();
}
}
60 changes: 60 additions & 0 deletions KimYeSeung/210930/Scripts/canscript.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
using System.Collections;
using System.Collections.Generic;
using UnityEngine;
using UnityEngine.SceneManagement;
public class canscript : MonoBehaviour

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change

여기에 공백은 왜 있는 건가요...?

{
Animator anim;
Vector3 movement;
Rigidbody2D rigid;
SpriteRenderer spriteRenderer;
BoxCollider2D boxcollider;
public GameObject player;

public AudioClip DioItem;
AudioSource audioSource;

float canMoveX = -150;
float canMoveY = 84;



void EatCan(){
canMoveX = -90;
canMoveY = 260;
}

void Awake()
{
rigid = GetComponent<Rigidbody2D>();
anim = GetComponent<Animator>();
spriteRenderer = GetComponent<SpriteRenderer>();
boxcollider = GetComponent<BoxCollider2D>();
player = GameObject.Find("player");
this.audioSource=GetComponent<AudioSource>();
}

void PlaySound(string action) {
switch(action){
case "ITEM":
audioSource.clip = DioItem;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

음악 실행을 여러 스크립트에서 실행하고 있네요. 음악 실행만 해주는 스크립트를 하나 만들어서 사용하는 것이 나중에 스크립트가 많아지거나 코드를 편집할 때 편리합니다.

break;
}
audioSource.Play();

}

public void gone(){
EatCan();
PlaySound("ITEM");
}

void OnTriggerEnter2D(Collider2D collision) {
//플레이어 떨어짐
transform.position = new Vector3(canMoveX,canMoveY,-1);

}
}


116 changes: 116 additions & 0 deletions KimYeSeung/210930/Scripts/mousemove.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,116 @@
using System.Collections;
using System.Collections.Generic;
using UnityEngine;
using UnityEngine.SceneManagement;

public class mousemove : MonoBehaviour
{
public float movePower = 1f;
public int nextMove;
public float StandBy;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

유니티 에디터에서 입력하기 위해서 public변수를 사용하는 거라면 [SerializeField] private를 대신 사용해 보세요. private처리가 되지만 유니티 에디터에서 읽고 쓰기가 가능해서 보안에 좋습니다.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tyrosine1153 우리는 이것을 보안이 아닌 '캡슐화' 라고 하기로 했습니다. 어차피 메모리 뜯으면 다 나오는 정보라서 보안보다는 캡슐화의 개념에 더 가깝습니다.

Animator anim;
Vector3 movement;
Rigidbody2D rigid;
SpriteRenderer spriteRenderer;
BoxCollider2D boxcollider;


void Awake()
{
gameObject.SetActive(false);
rigid = GetComponent<Rigidbody2D>();
Invoke("Think", 5);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

해당 Invoke 메소드의 경우 유니티 내에서 부하를 상당히 많이 잡아먹는 방식인데, 유니티에서 지원하는 Coroutine을 이용하면 훨씬 가벼운 지연 실행 처리가 가능한데, 이를 확장성있게 개발하는 방식에 대해서는 직접 C#과 Unity를 깊게 공부하는 것을 추천합니다.

anim = GetComponent<Animator>();
spriteRenderer = GetComponent<SpriteRenderer>();
boxcollider = GetComponent<BoxCollider2D>();
Invoke("start",StandBy);
}

void Update()
{
float h = nextMove;

//움직임 애니메이션
if(h>0||h<0){
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

h>0||h<0같은 경우 h != 0로 바꾸는 게 직관적일 것 같습니다.

anim.SetBool("moving", true);
}
else{
anim.SetBool("moving", false);
}



}


void FixedUpdate()
{
rigid.velocity = new Vector2(nextMove*movePower, rigid.velocity.y);

//낭떠러지 감지
Vector2 frontVec = new Vector2(rigid.position.x + nextMove*0.5f , rigid.position.y - 0.5f);
Debug.DrawRay(frontVec, Vector3.down, new Color(0, 1, 0));
RaycastHit2D rayHit = Physics2D.Raycast(frontVec, Vector3.down, 1, LayerMask.GetMask("Platform"));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

frontVec, rayHit과 같이 변수로 따로 선언해서 사용하는 부분 좋습니다.
DrawRay에서 new Color(0, 1, 0)는 미리 선언한 변수나 Color클래스의 정적 변수를 사용하는 것이 더 효율적이겠네요.
Color 클래스에 대한 문서 읽어보세요. Unity Docs - Color

if(rayHit.collider == null){
Turn();
}
}

void OnCollisionEnter2D(Collision2D collision){
if(collision.gameObject.tag == "Enemy"){
Turn();
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

collision.gameObject.tag == "Enemy" 부분은 GameObject의 함수인 CompareTag를 이용해 collision.gameObject.CompareTag("Enemy") 로 사용하는 것이 더 성능이 좋습니다.
설명은 귀찮으니 다른 글로 대체하겠습니다. [CompareTag vs Tag == ""]

}


//재귀 함수
void Think(){
nextMove = Random.Range(-1, 2);

float nextThinkTime = Random.Range(1f,3f);

Invoke("Think", nextThinkTime);

//방향
if(nextMove != 0){
spriteRenderer.flipX = nextMove == 1;
}

}

void Turn(){
nextMove = nextMove * -1;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

*=연산자를 사용해 nextMove *= -1로 하기
추천! 합니다.

spriteRenderer.flipX = nextMove == 1;

if(spriteRenderer.flipY != true){
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if(spriteRenderer.flipY != true){
if (!spriteRenderer.flipY){

bool 형식에는 굳이 비교연산자를 쓸 필요가 없답니다.

CancelInvoke();
}

Invoke("Think", 2);
}

public void OnDamaged(){
//죽음
Debug.Log("전달");

spriteRenderer.color = new Color(1,1,1,0.4f);

spriteRenderer.flipY = true;

boxcollider.enabled = false;

rigid.AddForce(Vector2.up * 5, ForceMode2D.Impulse);

Invoke("DeActive", 3);
}

void DeActive(){
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

과한 기능입니다. DeActive()gameObject.SetActive(false) 한줄을 랩핑할거라면 의미가 없습니다...

gameObject.SetActive(false);
}

void start(){
gameObject.SetActive(true);
}


}
Loading