Comments for review #2

Brunni132 · 2018-08-31T09:19:45Z

各コメントは // TODO Florian という接頭辞が付きます。

日本語を出来なくてすみません。もし質問があれば、ぜひ聞いて下さい。

よろしくお願いします。

【他のコメント】

build/game.bundle.js は通常のブランチでコミットしない方が良いです。（GitでPushしたものは、いつまでもレポジトリーの履歴に保存されるので、サイズが増えます。バイナリファイルはサイズも大きいですし、何度も更新されるので、普段は遠慮します。） gh-pages というのブランチでは特別に大丈夫です、他の方法がありませんから。

kishi

（ここへの返事の書き方がわからず編集で追記しました。作法的に間違いだったらすみません！）

追加でコメントありがとうございます。
たしかに、意味もないし、場所を取るしよくないですよね。次からはコミットしないようにします！

その場合、ちょっとした疑問があります。
この先ずっと、git statusにbundle.jsが存在していて、stageにaddするとき、ちょっとノイズになるような気がしました。
（でも、gh-pagesにcommitするので、ignoreもできないのかな、と）

でも、それはIt is what it is.でしょうか？
or
何かよい管理方法（テクニック）などありますでしょうか？

kithit · 2018-09-03T01:12:42Z

src/GameObject/EffectObjectHit.ts

@@ -33,6 +33,7 @@ export default class EffectObjectHit extends EffectObject {
 	}
 	destroyOeder() {
 		//配列を消しただけだとPhaserのオブジェクトは消えないので、自分のPhaserオブジェクトはすべて消すこと。
+		// TODO Florian -- そういえば、基本は作ったオブジェックとに消える機能の責任も渡します。


ありがとうございます！
その通りですね。考えが甘かったので、目が覚めました。

それぞれのサブクラスが、どのようなオブジェクト（例えばSpriteやemitter）をいくつ持つのかがわからないので、
わかっている本人の中でオブジェクトを消していましたが、これだとマネージャーの役割が果たせていないですね。。。（反省）

各オブジェクトを消すメソッドをマネージャーから呼んでもらう仕組みにしようと思います。

（別の方法として、次のものも考えましたが、↑の方がいいかなと思いました。
　
　Arrayなどで各オブジェクトを、Managerに渡す。
　Managerのdestroyでそれらのオブジェクトを消す。）

実際に組んでみようと思います。

src/ContinueScene.ts

src/GameObject/EffectObject.ts

kithit · 2018-09-03T01:20:56Z

src/GameObject/EffectObject.ts

@@ -27,6 +28,7 @@ export default class EffectObject {
 	}

 	destroyOeder() {
-		this.effectObjectManager.destroy(this.effectObjectManager.effectObjects.indexOf(this));
+		// TODO Florian -- As we talked before, I'd rather pass `this`.
+		this.effectObjectManager.destroyEffectObject(this);


コードに反映が間に合わずすみませんでした。
ありがとうございます。次からこのようにします！

こうしたほうが良い理由は
・その方が指し示す物（参照）が明確になるし、処理負荷も変わらないから。
ということですか？

すみません、英語で説明させていただきます。

Yes it's better that you avoid accessing properties (members) from another object if you can, so that if you change the other object, you have less chances that everything breaks (example if you rename EffectObjectManager.effectObjects or make it another kind of list).

ありがとうございます。理解できました。

今、これをもとに別のゲームを作っているのですが、
話を聞いてからコードを書き直すと、よく意味がわかりました。

あと、インターンで英語の重要性が身に染みたので、改めて勉強しています。
なので、（contextがわかる）英語を読む機会が増えてうれしいです。

kithit · 2018-09-03T01:38:38Z

src/GameObject/EffectObjectManager.ts

@@ -35,8 +35,16 @@ export default class EffectObjectManager {
 	}

 	//オブジェクトの参照が無くても番号で殺せるようにした
-	destroy(index: number) {
+	// TODO Florian -- Avoid using `destroy` as a name, since it's used in Phaser to mean "destroy this object" (not a child object)


そうだったんですね。わかりました。今後使わないようにします。
名前のサンプルもありがとうございます。

src/GameObject/EnemiesBase.ts

kithit · 2018-09-03T01:46:51Z

src/GameObject/EnemiesBase.ts

@@ -27,6 +27,8 @@ export default class EnemiesBase {
 		this.SoundSystem = SoundSystem;
 	}

+	// TODO Florian -- nice. You want to create an abstract class, and an abstract method here.
+	// https://medium.com/@pagalvin/looking-at-abstract-classes-and-methods-in-typescript-9769de98f65b


こういう設計にしたいなら、抽象クラスにしたほうがいいということですよね。

abstract class SampleClass {
abstract getString(): string;
}

kithit · 2018-09-03T01:49:16Z

src/GameObject/EnemiesEbihurai.ts

@@ -26,6 +26,7 @@ export class EnemiesEbihurai extends EnemiesBase {
 		// console.log("angle " + this.sprite.angle);
 	}

+	// TODO Florian -- not used. Also, since you also have it in the Shark, you could create another module, or put it in EnemiesBase so that everyone has access.


勉強にと思って翻訳したので、高見さんがみたときのため、残しておきます。

「このクラスでは使っていないので削除したほうがいいです。
使うとしても、シャークなどでも使っているので、クラスごとにっ書くのではなく、一つのモジュールとして独立させ、必要なクラスがそれぞれ参照するような形にもできます（そうしたほうがいいというニュアンスだと思います）」

kithit · 2018-09-03T01:53:49Z

src/GameObject/EnemiesShark.ts

@@ -21,6 +21,7 @@ export class EnemiesShark extends EnemiesBase {
 		this.gameUi = gu;
 		//music
 		this.Music = soundSystem;
+		// TODO Florian -- no need here! (executed upon startup)


to高見さん。これも翻訳残しておきます。

「ここには必要ない。起動時に実行される（ため？ってことかな）」。

確かに、シャークのコンストラクタとは役割が違う機能なので、
今の設計だったらGameSceneなどで書いたほうがよかったかもですね。
作業の競合をよけるためこうしてくれたのだと思いますが。

本当はSceneManager等を作ったほうがよかったですね（チーム全体として）

大きい問題ではないので、そのままでマージして欲しいなら、教えてください！
(上記のコメントのところも）

kithit · 2018-09-03T02:05:27Z

src/GameObject/EnemiesShark.ts

@@ -60,6 +61,7 @@ export class EnemiesShark extends EnemiesBase {
 	}

 	addDamage() {
+		// TODO Florian -- note that with inheritance, you may also call the parent addDamage method (in EnemiesBase). Use super.addDamage().


to：高見さん
「メモ：継承では、継承元のメソッドを呼ぶことができます）
super.addDamage();
という風に使えます」

EffectObjectで使っている機能なのでサンプルになるかもしれません。

kithit · 2018-09-03T02:08:28Z

src/GameObject/EnemiesShark.ts

@@ -92,6 +94,7 @@ export class EnemiesShark extends EnemiesBase {
 				this.mode = 0;
 			}
 		}
+		// TODO Florian -- good, but you may use constants instead of numbers so that your states are easier to understand (see enum)


to:高見さん
「いいですね。
　0として扱っている状態を、0ではなく定数を使うと、状態がわかりやすくなります（enumという仕組みを確認して見てください）」

これもeffectObjectで使っている機能なので参考になると思います。

EffectObjectType.hogehoge
という感じで、使っています。

kithit · 2018-09-03T02:13:11Z

src/GameScene.ts

@@ -89,6 +89,7 @@ export default class GameScene extends TimesteppedScene {

 		this.game.camera.x += this.scrollSpeed;

+		// TODO Florian -- good structure, but usually if you get a dt, you should pass it down (it's there for a reason, try on a slow machine)


ありがとうございます。
処理が遅くなることは体感しているので、改善したいです。

ただ、具体的な理解ができていません。

＞" usually if you get a dt,"
dtを渡すというのは「uptdate()を呼んでいるなら」、ということでしょうか。

引数でdtを渡しているわけではないので、フローリアンさんの指摘が「何を指しているのか」「どうしたらいいのか」の理解ができませんでした。

自分でも、example等を見て、他プロジェクトがどうしているのか見てみようと思います。

ありがとうございます。

メールで返信しましたが、fixedUpdate() は dt というパラメターを渡されていますので、その dt を、物理学の計算、全的に使うと良いと思う。他のオブジェクトでも使われます。

丁寧にありがとうございます！　理解しました。

「処理落ち」の話かと勘違いしていました。
「処理速度が違うＰＣ（環境）でも、同じ速度でゲームが進行するように、dtを使う」という話だったのですね。

ありがとうございます。

kithit · 2018-09-03T02:31:39Z

src/ScoreManager.ts

@@ -1,3 +1,5 @@
+// TODO Florian -- avoid naming functions like that (a manager is usually a class, and when seeing ScoreManager, we
+// don't understand that you are returning the high score).


To：高見さん
「この設計において、こういう関数の名前を付けないでください。
（マネージャは通常はクラスですが、ScoreManagerはそうではないので。
　//また、その役割も名前と合っていないようです（高いスコアを返している）」結構意訳です。

私もできてないことが多いですが、クラス、関数、変数の命名はもっとその中身と一致させる事が重要な技術の一つとだと今回強く学びました！

Brunni132 · 2018-09-05T09:55:01Z

手寧のレビューありがとうございます！

…め。敵と自分の移動速度差ができてしまう）

Comments for review

6f35eeb

Brunni132 force-pushed the review/comments branch from 857bf48 to 6f35eeb Compare August 31, 2018 09:22

kithit reviewed Sep 3, 2018

View reviewed changes

src/ContinueScene.ts Show resolved Hide resolved

kithit reviewed Sep 3, 2018

View reviewed changes

src/GameObject/EffectObject.ts Show resolved Hide resolved

kithit reviewed Sep 3, 2018

View reviewed changes

src/GameObject/EnemiesBase.ts Show resolved Hide resolved

kithit reviewed Sep 3, 2018

View reviewed changes

kithit added 3 commits September 3, 2018 12:21

使っていないオブジェクトを削除

1161b86

使っていないオブジェクトの削除漏れ。

ec6ca6c

オブジェクトが自分の子要素（sprite）などを自分で消さず、消す機能をマネージャーに開示して叩いてもらうように変更。

7a1a5a5

プレイヤーとバレットの移動にDeltaTimeを適用させて、ゲーム進行速度がFPSの影響を受けないようにした（ほんとは敵もやらないと絶対だ…

465fea3

…め。敵と自分の移動速度差ができてしまう）

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Comments for review #2

Comments for review #2

Brunni132 commented Aug 31, 2018 •

edited by kithit

Loading

kithit Sep 3, 2018

kithit Sep 3, 2018 •

edited

Loading

Brunni132 Sep 5, 2018

kithit Sep 6, 2018

kithit Sep 3, 2018

kithit Sep 3, 2018

kithit Sep 3, 2018 •

edited

Loading

kithit Sep 3, 2018 •

edited

Loading

Brunni132 Sep 5, 2018

kithit Sep 3, 2018

kithit Sep 3, 2018

kithit Sep 3, 2018

Brunni132 Sep 5, 2018

kithit Sep 6, 2018

kithit Sep 3, 2018

Brunni132 commented Sep 5, 2018

		@@ -89,6 +89,7 @@ export default class GameScene extends TimesteppedScene {

		this.game.camera.x += this.scrollSpeed;

		// TODO Florian -- good structure, but usually if you get a dt, you should pass it down (it's there for a reason, try on a slow machine)

		@@ -1,3 +1,5 @@
		// TODO Florian -- avoid naming functions like that (a manager is usually a class, and when seeing ScoreManager, we
		// don't understand that you are returning the high score).

Comments for review #2

Are you sure you want to change the base?

Comments for review #2

Conversation

Brunni132 commented Aug 31, 2018 • edited by kithit Loading

【他のコメント】

kishi

Choose a reason for hiding this comment

kithit Sep 3, 2018 • 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

kithit Sep 3, 2018 • edited Loading

Choose a reason for hiding this comment

kithit Sep 3, 2018 • 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

Brunni132 commented Sep 5, 2018

Brunni132 commented Aug 31, 2018 •

edited by kithit

Loading

kithit Sep 3, 2018 •

edited

Loading

kithit Sep 3, 2018 •

edited

Loading

kithit Sep 3, 2018 •

edited

Loading