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

Add argument RunnerStartParameters of Runner#start() #272

Merged
merged 13 commits into from
Aug 12, 2021

Conversation

ShinobuTakahashi
Copy link
Contributor

概要

Runner#start() に引数 { paused?: boolean } を追加します。
paused が真の場合はコンテンツの進行を停止した状態で g.game のインスタンスを返します 。

@@ -138,6 +138,25 @@ describe("Runner の動作確認 (v1)", () => {

runner.stop();
});

xit("Runner#start({paused:true}) でコンテンツは進行しない", async () => {
Copy link
Contributor Author

@ShinobuTakahashi ShinobuTakahashi Aug 4, 2021

Choose a reason for hiding this comment

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

xit でスキップとしていますが、ローカルでは it でテストが通る事を確認しています。
itに直すのは CI環境 で確認したいので、別のタスクとさせてください。

@ShinobuTakahashi ShinobuTakahashi requested review from xnv and yu-ogi August 4, 2021 05:43
@ShinobuTakahashi ShinobuTakahashi added the enhancement New feature or request label Aug 4, 2021
Comment on lines 26 to 34
if (!paused) {
this.running = true;
game = await this.startGameDriver();
} else {
this.running = false;
this.platform?.pauseLoopers(); // コンテンツ開始前に停止する
this.driver!.startGame();
game = this.driver!._game;
}
Copy link
Member

@xnv xnv Aug 4, 2021

Choose a reason for hiding this comment

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

他のコメントの議論とコンフリクトしそうな話になりますが、そもそもこんな難しい形の実装になってしまうんですっけ……?

  1. initGameDriver() は常に pauseLoopers() する
  2. このメソッドは !paused なら resumeLoopers() する

というだけの話では済まないでしょうか? startGame() する箇所が二箇所に増えるとか、できれば避けたいように思えますが。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

別途ご相談とさせてください。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

別途ご相談させていただいた内容で修正しました。

@ShinobuTakahashi
Copy link
Contributor Author

CI 環境で xit を外したテストが正常終了することを数回実行し確認。

@@ -210,7 +213,9 @@ export class RunnerV1 extends Runner {
reject(e);
return;
}
this.pause();
Copy link
Member

Choose a reason for hiding this comment

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

initialize() を挟むと非同期が絡むので、 platform の生成直後に pause() してしまいたいです。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

platform 生成直後に this.pause() するよう修正しました。
生成直後だと Platform#createLooper() がまだ実行されないため、isLooperPaused フラグを追加し、createLooper() で looper 生成時に stop 状態なら、debugStop() した looper を返すように修正しました。

Copy link
Member

Choose a reason for hiding this comment

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

はい。Runner が looper の生成タイミングに依存するべきではないので、適切な対応だと思います。

Copy link
Member

@xnv xnv left a comment

Choose a reason for hiding this comment

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

コメントつけていますが approved.

@@ -22,6 +22,14 @@ export interface RunnerParameters {
externalValue?: { [key: string]: any };
}

export interface RunnerStartParameters {
/**
* コンテンツの進行を停止するかどうか。
Copy link
Member

Choose a reason for hiding this comment

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

https://github.com/akashic-games/headless-driver/pull/272/files#diff-774b124f6346a73abd8982e4cb5c517120b3e4b1c87f71cc47f7bf8a941a0430R123-R130

「停止」は stop() に対応しているので、 pause() に合わせて「一時停止」にしたいです。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"一時停止" に修正しました。

@@ -233,7 +237,11 @@ export class RunnerV3 extends Runner {
reject(e);
return;
}
if (!result) {
return reject(new Error("Game is null."));
Copy link
Member

@xnv xnv Aug 11, 2021

Choose a reason for hiding this comment

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

内部動作的には、 configurationUrl を渡しているのでこのパスに来ることはないはずです。「 GameDriver の内部実装上ここに来ることはないはずだが念のため確認」というコメントをつけておきたいです。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

コメント追加しました。

Copy link
Contributor

@yu-ogi yu-ogi left a comment

Choose a reason for hiding this comment

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

  • (念の為) 機能追加なのでマイナーバージョンの更新でお願いします。
  • PullRequestのタイトルの再確認をお願いします。

@ShinobuTakahashi ShinobuTakahashi changed the title Add argumnt to Runner#start() Add argument RunnerStartParameters of Runner#start() Aug 11, 2021
@ShinobuTakahashi ShinobuTakahashi merged commit 4e92005 into master Aug 12, 2021
@ShinobuTakahashi ShinobuTakahashi deleted the add-paused-argumnt-to-start branch August 12, 2021 01:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants