Kim Ye Seung, 21.09.30 #8

maybeso0618 · 2021-09-30T08:22:59Z

새끼 고양이가 츄르를 얻기 위해 쥐들을 피해 캣타워의 정상으로 향하는 플랫포머 게임

게임 내 요소

고양이 : 좌우 이동 및 점프, 높은 곳에서 덜어 질시 평지에 닿기 전까지 좌우 이동 불가, HP는 기본 3개이며 특정 이스터에그 아이템을 획득시 목숨이 7개로 중가
참치캔 : HP 회복
이스터에그 : 맵 구석에 존재하며 획득시 고양의 HP가 증가
쥐 : 고양이를 방해하는 적, 닿는 경우 고양이의 HP 감소, 밟는 경우 쥐 사망

스크립트

BGM.cs : 배경음악 관리(효과음은 playermove.cs가 관리)
canscript.cs : 참치캔을 획득시 효과음을 발생시키며 다음 지점으로 이동
Gamemanager.cs : 게임 시스템 관리
mousemove.cs : 쥐 오브젝트 관리
playermove.cs : 고양이(플레이어) 오브젝트 관리
sevenHP.cs : 이스터에그 아이템 관리1

tyrosine1153 · 2021-10-03T07:29:31Z

누가 썼는지 설명 형식이 아주 깔끔하고 예쁘군요.

tyrosine1153

스크립트는 세개밖에 안봤는데요. 나머지는 나중에 해보겠습니다. 지적한 내용은 다음과 같이 요약할 수 있습니다.

명명 규칙에 대해서 더 알아보고 지켜주세요.
명명할때의 형식을 정해서 형식에 맞게 통일해 주세요.
자주 쓰는 상수 값은 변수로 지정해 사용해 주세요. 배열, 반복문을 사용하세요.
C#의 코딩 스타일에 대해 정리한 블로그 글을 달겠습니다. 읽어보고 참고하세요. coding-guide

tyrosine1153 · 2021-10-03T07:34:13Z

KimYeSeung/210930/Scripts/BGM.cs

+using UnityEngine.SceneManagement;
+
+public class BGM : MonoBehaviour
+{


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

tyrosine1153 · 2021-10-03T07:40:23Z

KimYeSeung/210930/Scripts/BGM.cs

+    public AudioClip BGM1;
+    public AudioClip BGM2;
+    public AudioClip BGM3;
+    AudioSource audioSource;


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

public AudioClip[] BGMClips = new AudioClip[3]; ... audioSource.clip = BGMClips[0];

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

tyrosine1153 · 2021-10-03T07:44:52Z

KimYeSeung/210930/Scripts/BGM.cs

+    public GameObject player;
+
+    Rigidbody2D rigid;
+


public은 public 끼리, private는 private끼리. 변수들도 비슷한 것끼리 놓아야 보는 사람이 편하고 좋습니다.

AudioSource audioSource에서 audioSource는 자료형 그대로 쓰고 Rigidbody2D rigid에서 rigid는 줄여 쓰는 데에 이유가 있나요? 둘 다 unity component이니까 통일해 주는 것을 추천합니다.

private도 생략하지 않는 것을 추천합니다.

tyrosine1153 · 2021-10-03T07:47:30Z

KimYeSeung/210930/Scripts/BGM.cs

+    void Awake() {
+        player = GameObject.Find("player");
+        this.audioSource=GetComponent<AudioSource>();
+        rigid = GetComponent<Rigidbody2D>();


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

tyrosine1153 · 2021-10-03T07:51:20Z

KimYeSeung/210930/Scripts/BGM.cs

+    }
+
+    void PlaySound(string action) {
+        switch(action){


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

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

로 줄일 수 있겠네요.

tyrosine1153 · 2021-10-03T08:15:29Z

KimYeSeung/210930/Scripts/Gamemanager.cs

+        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);


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

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

tyrosine1153 · 2021-10-03T08:16:35Z

KimYeSeung/210930/Scripts/Gamemanager.cs

+        else{
+            //플레이어 죽음
+            --health;
+            UIHp[health].color = new Color(1,0,0,0.4f);


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

tyrosine1153 · 2021-10-03T08:18:19Z

KimYeSeung/210930/Scripts/Gamemanager.cs

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


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

tyrosine1153 · 2021-10-03T08:19:21Z

KimYeSeung/210930/Scripts/Gamemanager.cs

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


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

tyrosine1153 · 2021-10-03T08:24:12Z

KimYeSeung/210930/Scripts/canscript.cs

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


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

tyrosine1153

다른 리뷰와 비슷한 내용이 많았고 그런 것들은 따로 적지 않았습니다. 그래서 지적이 필요한 부분은 모두 표시하지 않았으니 중복되는 부분은 찾아서 봐주세요.

tyrosine1153 · 2021-10-03T09:16:34Z

KimYeSeung/210930/Scripts/mousemove.cs

+{
+    public float movePower = 1f;
+    public int nextMove;
+    public float StandBy;


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

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

tyrosine1153 · 2021-10-03T09:18:08Z

KimYeSeung/210930/Scripts/mousemove.cs

+        float h = nextMove;
+
+        //움직임 애니메이션
+        if(h>0||h<0){


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

tyrosine1153 · 2021-10-03T09:22:22Z

KimYeSeung/210930/Scripts/mousemove.cs

+        //낭떠러지 감지
+        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"));


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

tyrosine1153 · 2021-10-03T09:25:28Z

KimYeSeung/210930/Scripts/mousemove.cs

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


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

tyrosine1153 · 2021-10-03T09:28:26Z

KimYeSeung/210930/Scripts/mousemove.cs

+    }
+
+    void Turn(){
+        nextMove = nextMove * -1;


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

tyrosine1153 · 2021-10-03T09:33:26Z

KimYeSeung/210930/Scripts/playermove.cs

+
+      }
+
+      void PlaySound(string action) {


string 연산은 대게 성능이 좋지 않습니다. 문자를 통해 직관적으로 값을 보고 싶다면, Enum 자료형을 정의해 만드는 것을 추천합니다. Enum 이란?
Enum 자료형은 구성 값들이 기본적으로 정수에 대응되기 때문에 연산이 간단하고 정수마다 이름을 붙이는 것과 비슷해서 코드를 읽는 사람이 직관적으로 이해할 수 있습니다. 위에 링크 단 글 꼭 읽어보세요.

tyrosine1153 · 2021-10-03T09:41:17Z

KimYeSeung/210930/Scripts/playermove.cs

+            //게임 재시작
+            if(Input.GetKeyDown(KeyCode.R)){
+                  SceneManager.LoadScene(0);
+            }


플레이어의 동작 부분 외에 요소는 다른 스크립트에서 나누어 처리하는 것이 나중에 수정할 때 용도에 맞게 코드를 찾기 쉽습니다.

tyrosine1153 · 2021-10-03T09:42:58Z

KimYeSeung/210930/Scripts/sevenHP.cs

+
+    public float turnSpeed;
+    public playermove Player;
+    public GameObject player;


안 쓰는 코드는 제 때 지워주세요.

sonohoshi

전체적으로 C#의 문법, Unity의 기본 API에 대해 지식이 부족한 경향을 보입니다.
Unity의 특징, C#을 이용하는 부분에 대한 기본기를 기르도록 합시다.

sonohoshi · 2021-10-04T03:39:20Z

KimYeSeung/210930/Scripts/BGM.cs

+    public AudioClip BGM3;
+    AudioSource audioSource;
+    public playermove Player;
+    public GameObject player;


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

sonohoshi · 2021-10-04T03:42:12Z

KimYeSeung/210930/Scripts/BGM.cs

+    Rigidbody2D rigid;
+
+    void Awake() {
+        player = GameObject.Find("player");


Suggested change

player = GameObject.Find("player");

player = Transform.FindWithTag("player").gameObject;

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

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

sonohoshi · 2021-10-04T03:45:15Z

KimYeSeung/210930/Scripts/BGM.cs

+
+    }
+
+    public void Ms1(){


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

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

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

sonohoshi · 2021-10-04T03:48:41Z

KimYeSeung/210930/Scripts/Gamemanager.cs

+        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);


@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;

sonohoshi · 2021-10-04T03:49:45Z

KimYeSeung/210930/Scripts/Gamemanager.cs

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


Suggested change

UIPoint.text = (totalPoint + stagePoint).ToString();

UIPoint.text = $"{totalPoint + stagePoint}";

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

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

sonohoshi · 2021-10-04T04:15:38Z

KimYeSeung/210930/Scripts/playermove.cs

+                  JumpCount = 0;
+            }
+
+            if(rigid.velocity.y < -20){


-20 이라는 값에 대해 상수 처리를 해주세요.

sonohoshi · 2021-10-04T04:20:30Z

KimYeSeung/210930/Scripts/playermove.cs

+            }
+
+            //방향전환
+            if(Input.GetButton("Horizontal")){


Suggested change

if(Input.GetButton("Horizontal")){

var axis = Input.GetAxisRaw("Horizontal");

if(axis != 0)

{

spriteRenderer.flipX = axis < 0

}

이러면 Input 관련 메소드를 두번씩이나 쓸 필요가 없고 axis 라는 값 하나로 스프라이트를 뒤집을지까지 결정할 수 있습니다.

sonohoshi · 2021-10-04T04:21:06Z

KimYeSeung/210930/Scripts/playermove.cs

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


Suggested change

anim.SetBool("moving", false);

anim.SetBool("moving", false);

탭은 어디갔나요?

sonohoshi · 2021-10-04T04:22:06Z

KimYeSeung/210930/Scripts/playermove.cs

+                        timecheck = 0;
+                  }        
+            }
+            if(rigid.velocity.y < -1){


위의 if문과 같은 조건을 처리하는데 왜 블록이 구분되어있나요?

sonohoshi · 2021-10-04T04:23:17Z

KimYeSeung/210930/Scripts/playermove.cs

+      void OnCollisionEnter2D(Collision2D collision){
+
+             //hit
+            if(collision.gameObject.tag == "Enemy"){


Suggested change

if(collision.gameObject.tag == "Enemy"){

if(collision.gameObject.CompareTag("Enemy")){

use CompareTag

Add file KimYe-seung

c81a799

tyrosine1153 reviewed Oct 3, 2021

View reviewed changes

sonohoshi reviewed Oct 4, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Kim Ye Seung, 21.09.30 #8

Kim Ye Seung, 21.09.30 #8

maybeso0618 commented Sep 30, 2021

tyrosine1153 commented Oct 3, 2021 •

edited

Loading

tyrosine1153 left a comment •

edited

Loading

tyrosine1153 Oct 3, 2021

tyrosine1153 Oct 3, 2021

tyrosine1153 Oct 3, 2021

tyrosine1153 Oct 3, 2021

tyrosine1153 Oct 3, 2021

tyrosine1153 Oct 3, 2021

tyrosine1153 Oct 3, 2021

tyrosine1153 Oct 3, 2021

tyrosine1153 Oct 3, 2021

tyrosine1153 Oct 3, 2021

tyrosine1153 left a comment

tyrosine1153 Oct 3, 2021

sonohoshi Oct 4, 2021

tyrosine1153 Oct 3, 2021

tyrosine1153 Oct 3, 2021

tyrosine1153 Oct 3, 2021

tyrosine1153 Oct 3, 2021

tyrosine1153 Oct 3, 2021

tyrosine1153 Oct 3, 2021

tyrosine1153 Oct 3, 2021

sonohoshi left a comment

sonohoshi Oct 4, 2021

sonohoshi Oct 4, 2021

sonohoshi Oct 4, 2021

sonohoshi Oct 4, 2021

sonohoshi Oct 4, 2021

sonohoshi Oct 4, 2021

sonohoshi Oct 4, 2021

sonohoshi Oct 4, 2021

sonohoshi Oct 4, 2021

sonohoshi Oct 4, 2021

sonohoshi Oct 4, 2021

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

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

-            if(Input.GetButton("Horizontal")){
+            var axis = Input.GetAxisRaw("Horizontal");
+            if(axis != 0)
+            {
+                spriteRenderer.flipX = axis < 0
+            }

	anim.SetBool("moving", false);
	anim.SetBool("moving", false);

	if(collision.gameObject.tag == "Enemy"){
	if(collision.gameObject.CompareTag("Enemy")){

Kim Ye Seung, 21.09.30 #8

Are you sure you want to change the base?

Kim Ye Seung, 21.09.30 #8

Conversation

maybeso0618 commented Sep 30, 2021

게임 내 요소

스크립트

tyrosine1153 commented Oct 3, 2021 • edited Loading

tyrosine1153 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tyrosine1153 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonohoshi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tyrosine1153 commented Oct 3, 2021 •

edited

Loading

tyrosine1153 left a comment •

edited

Loading