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 AMFlowStore/AMFlowClient putStartPoint Trigger #263

Merged
merged 13 commits into from
Jul 1, 2021

Conversation

kamakiri01
Copy link
Contributor

@kamakiri01 kamakiri01 commented Jun 9, 2021

このpull requestが解決する内容

概要

  • AmflowStore を interface にし、既存の AmflowStore 実装を MemoryAMFlowStore にリネームします。
  • AMFlowClientManager#createAMFlowStore で生成していた AmflowStore をやめ、外部から factory を渡せるように変更します。
  • AmflowStore#onPutStartPoint を追加します。

PRコメントで議論後方針を修正しました。

  • AMFlowStore#putStartPointTrigger を追加し、これを基点に発火する AMFlowClient#onPutStartPoint を追加します。

機能追加のためマイナーバージョンが上がります。

破壊的な変更を含んでいるか?

  • なし

playId: string;
sendEventTrigger: Trigger<Event> = new Trigger();
sendTickTrigger: Trigger<Tick> = new Trigger();
onPutStartPointTrigger: Trigger<number> = new Trigger();
Copy link
Contributor Author

@kamakiri01 kamakiri01 Jun 9, 2021

Choose a reason for hiding this comment

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

プレイ中の一意な識別子として frame: number を使う想定です。
devtoolに一覧を出すためにはstartPoint全体を送る必要はなく、識別子だけで良いだろうという意図ですが、他の方針もあると思います。コメント頂ければと思います。

Copy link
Member

Choose a reason for hiding this comment

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

細かい情報は個別に GET する API を作って問い合わせればいいので、さほど慎重にならなくてよさそうです。ただまあ { frame, timeStamp } ぐらい送ってもいいかなと思います。(手動ティックだと frame がほとんど情報にならないので) オブジェクトにしておけば後でプロパティが増えても影響が小さいというのもあり。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

interface StartPointHeader {
	frame: number;
	timestamp: number;
}

を型定義してこれを送るようにします。

@kamakiri01 kamakiri01 requested a review from xnv June 11, 2021 03:02
playId: string;
sendEventTrigger: Trigger<Event> = new Trigger();
sendTickTrigger: Trigger<Tick> = new Trigger();
onPutStartPointTrigger: Trigger<number> = new Trigger();
Copy link
Member

Choose a reason for hiding this comment

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

周囲に合わせるなら putStartPointTrigger かと思います。逆にここで初めて公開する名前なので、ここで onSendTick, onPutStartPoint などと改名してしまってもよさそうです。(〜Trigger は初期の pdi-browser に合わせたもの、 on〜 は AEv3 に合わせたもの、 on〜Trigger は前例がない認識)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

確かに、on~ に揃えます。

@kamakiri01 kamakiri01 requested a review from yu-ogi June 17, 2021 05:37
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.

通常の使い方だと利用されることがないので、できれば以下辺りで簡単なテストを追加しておきたいです。

// Play を resume した後に sendTick できる
activeAMFlow.sendTick([1]);
// Play を resume した後に sendEvent できる
passiveAMFlow.sendEvent([0, 1, "dummy-player-id"]);
// Play を resume した後に putStartPoint できる
activeAMFlow.putStartPoint(
{
frame: 10,
timestamp: 1000,
data: "hoge"
},
(e) => {
if (e) {
reject(e);
return;
}
resolve();
}
);

イメージ:

let startPoint;
activeAMFlow.onPutStartPoint.add((s) => { startPoint = s; });

// Play を resume した後に putStartPoint できる 
activeAMFlow.putStartPoint(
...
 	(e) => { 
 		if (e) { 
 			reject(e); 
 			return; 
 		} 
		expect(startPoint).toEqual(...);
 		resolve(); 
 	} 

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 のタイトルと説明を修正内容に合わせてください。

@kamakiri01 kamakiri01 changed the title replace AmflowStore to interface add AMFlowStore/AMFlowClient putStartPoint Trigger Jun 28, 2021
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の説明にある

AMFlowStore#putStartPoint Trigger を追加し、これを基点に発火する AMFlowStore#onPutStartPoint を追加します。

はそれぞれ

  • AMFlowStore#putStartPointTrigger
  • AMFlowClient#onPutStartPoint

が正しいかと思います。

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.

LGTM です。ご対応ありがとうございます。

@yu-ogi yu-ogi added the enhancement New feature or request label Jun 29, 2021
@kamakiri01 kamakiri01 merged commit 58bbbc9 into master Jul 1, 2021
@kamakiri01 kamakiri01 deleted the amflowstore-replace-interface branch July 1, 2021 11:31
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