feat: support 'prepare' phase before the scene load #456

yu-ogi · 2023-07-26T06:09:34Z

このpull requestが解決する内容

g.Scene のアセット読み込み後、任意の非同期処理を行うための prepare をサポートします。
g.Scene#vars を追加します。この値はゲームエンジンのロジックからは使用されず、ゲーム開発者が任意の値を代入することができます。

コードイメージは以下のようになります。

const scene = new g.Scene({ ... });
scene.onLoad.addOnce(() => {
  console.log(scene.vars.data);
});

g.game.pushScene(scene, {
  prepare: done => {
    someAsyncFunc()
      .then((data)=> {
        scene.vars.data = data;
        done();
       })
      .catch(_error => done());
  }
});

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

なし

xnv · 2023-07-26T07:58:09Z

src/__tests__/GameSpec.ts

+							sequence.push("scene1 prepared");
+							done();
+						},
+						100 // この値にとくに根據は無い


全くどうでもいいんですが、旧字体の「據」よりは常用漢字の「拠」にしときませんか。

a824ca7 ミスでした。修正済みです。

xnv · 2023-07-26T08:00:33Z

src/Game.ts

+	preserveCurrent?: boolean;
+	prepare?: (done: () => void) => void;


このあたりのプロパティにもコメントつけておきたいです。(ここにないと API リファレンスに prepare の説明が出るところがなさそうです)

fca39e8 こちらコメント追加しました。

xnv · 2023-07-26T08:15:42Z

src/Game.ts

 						this._doPushScene(req.scene, false);
 						break;
 					case PostTickTaskType.ReplaceScene:
 						// NOTE: replaceSceneの場合、pop時点では_sceneChangedをfireしない。_doPushScene() で一度だけfireする。
 						this._doPopScene(req.preserveCurrent, false, false);
+						if (req.prepare) {
+							this._interruptSceneLoading(req.prepare);


this.loadingScene をここで代入する形になっていますが、よく見ると _doPushScene() が引数で loadingScene 指定できるので、そちらで済まないでしょうか？

Game#loadingScene を書き換えると正しく戻せるかどうかかなり見極めが必要そうなので (特に pushScene() が同期的に複数回呼ばれた場合) 、書き換えなしで済むならその方がすっきりするかと思います。

a84007a おっしゃるとおり_doPushScene() を使うパターンでもテスト上問題なさそうだったため修正しました。

xnv · 2023-07-26T08:17:52Z

src/Game.ts

 						this._doPushScene(req.scene, false);
 						break;
 					case PostTickTaskType.ReplaceScene:
 						// NOTE: replaceSceneの場合、pop時点では_sceneChangedをfireしない。_doPushScene() で一度だけfireする。
 						this._doPopScene(req.preserveCurrent, false, false);
+						if (req.prepare) {
+							this._interruptSceneLoading(req.prepare);
+						}
 						this._doPushScene(req.scene, false);
 						break;
 					case PostTickTaskType.PopScene:


_doPopScene() の中にもローディングシーンを積み直すパスがありますが、この時 prepare が呼ばれるか確認したいです。(難儀そうな気がしています)

popScene() の際にも prepare を呼ぶ (待つ) べきでしょうか。念のため確認しておきたいです。

はい。当初考えていませんでしたが、状況によって待つ必要がありそうに思えています。(念のため: これは popScene() に引数を追加して popScene(scene, { prepare }) にするという話ではありません)

具体的には次のような状況を気にしています。

const scene1 = new g.Scene({ ..., assetIds: ["foo"] }); // なんらかのアセット読み込みが必要なシーン const scene2 = new g.Scene(...); // 適当なシーン g.game.pushScene(scene1, { prepare: (done) => { someAsyncOp().then(done); } }); g.game.pushScene(scene2); g.game.popScene();

ロード中のシーン (scene1) のロード完了を待たずに次のシーン (scene2) が push された状況では、pop して scene1 に戻る時にローディングシーンを積み直してロード完了を改めて待つ必要があります。(過去、この「pop 時に発生するロード待ち」ができておらず不具合となったことがあります)　このとき、prepare が指定されているなら呼ばないと不味そうです。

なるほど、確かにシーンロード中に新たに別のシーンをロードするケースは想定できていませんでした。対応を考えます。

4e782df にて対応しました。
g.Scene#_currentPrepare を新設し、g.Game#popScene() 時に g.Game がその中身を参照するように修正しています。

yu-ogi · 2023-08-02T08:58:48Z

src/Scene.ts

+		return (
+			this._sceneAssetHolder.waitingAssetsCount > 0 ||
+			(!!this._storageLoader && !this._storageLoader._loaded) ||
+			!!this._currentPrepare


読み込むアセットの無い scene に対して prepare を設定していた場合に prepare を待たず onLoad が発火してしまっていたため条件を変更しました。

xnv · 2023-08-04T11:16:51Z

src/Game.ts

+		});
+		// ローディングシーンを保持するためクロージャを許容
+		loadingScene.onTargetReady.addOnce(() => {
+			const done = loadingScene.end.bind(loadingScene);


これ、 prepare が外部の非同期処理を想定する関係上、 done が呼ばれた時に「 Scene や Game が既に破棄されていた状況」を想定しなければならないことに気づきました。

loadingScene#end そのまま渡すのではなく、「" Game が死んでいたら何もせず抜ける" などチェックをした上で end を呼ぶ関数」を使う必要がありそうです。

(類例として、非同期処理後に逐一破棄チェックしている GameDriver があります。あちらは正確には例外で強引に処理を潰してるのでいい状態でもありませんが)

49bd624 こちら対応しました。

xnv · 2023-08-08T06:17:01Z

src/Game.ts

+			explicitEnd: true,
+			name
+		});
+		// ローディングシーンを保持するためクロージャを許容


今更ですがこれコメント正しいでしょうか。クロージャで保持してるものは prepare のように見えます。

89d0fe9 こちら実態に沿うようにコメント修正しました。

xnv · 2023-08-08T06:45:19Z

src/Scene.ts

@@ -961,5 +980,6 @@ export class Scene implements StorageLoaderHandler {
 		if (this._loadingState === "loaded-fired") return;
 		this.onLoad.fire(this);
 		this._loadingState = "loaded-fired";
+		this._currentPrepare = undefined; // TODO: 本来は _currentPrepare に値を代入する側 (e.g. g.Game) でクリアすべき


TODO コメントになっていますが、これ _createPreparingLoadingScene() で prepare の代わりに Scene を引数にとり、 onTargetReady のハンドラで prepare を呼ぶ直前にクリアすればよいのではないでしょうか。

これには二つの意味があるように思えています:

a) _currentPrepare を触る箇所を (Game に) 限定できて嬉しい (TODO コメントどおり)

b) prepare() を呼ぶ前に _currentPrepare をクリアしないと、二重に呼ぶ懸念が残りそう

後者が問題になるケースとして考えているのは「 prepare() の呼び出し後 done() で返るまでの間にまた pushScene() 〜 popScene() が走った時」です。(アセットロード中に pushScene() されるケースを織り込む一方で、 prepare 中のそれを想定しないのは片手落ちなので)

ただこのケースは、真面目に考慮しはじめると大掛かりになりすぎる気もします。どちらかというと (もともとエッジケースを十分に考慮できていたとは言えない) explicitEnd の問題修正を始めることになってしまうので……。この PR では一旦「 prepare 中にシーンスタックを操作してはいけない」という制限事項を加えて済ませるのも手かと思います。

備忘: 危うそうなところ

prepare() の呼び出し後 done() までの間にまた pushScene() され (a)、さらにその後 popScene() された (b) 時、

i) (b) 時点で、 Scene#_currentPrepare が残っているともう一回 prepare を呼びそう

ii) (a) 時点では「 Game も対象の Scene も生きているが、自分 (LoadingScene) がスタックトップにいない」ので、ここで done() されるとまずそう

done() は LoadingScene#end() を呼び、そこで popSceneRaw() が走ってスタックトップから自身 (ローディングシーン) を取り除こうとする＝おそらくスタックが壊れる

しかし逆に LoadingScene#end() しないと、「 prepare が走って done() も呼ばれた」対象のシーンの onLoad を呼ぶ契機がなくなる

_createPreparingLoadingScene() で scene とprepare を指定するように修正しました。

呼び出し箇所が複数あり scene._currentPrepare への代入処理をまとめるため引数を分けました

prepare 中にシーンスタックを操作してはいけない という旨のコメントし、一旦は上記を制限事項とする方針にしました。

xnv · 2023-08-14T05:44:01Z

src/Game.ts

+				prepare(done);
+			} else {
+				// NOTE: 異常系ではあるが prepare が存在しない場合は loadingScene.end() を直接呼ぶ
+				this._postTickTasks.unshift({


これ unshift が必要でしょうか？　_pushPostTickTask() でよさそうですが。どっちみち相当な異常系なのでこだわりませんが、事情があるならコメントしておきたいように思います。

04ff1b3 こちら修正しました。

xnv

コメント加えていますが approved.

yu-ogi added 3 commits July 26, 2023 14:51

feat: add g.Scene#vars

813ce6d

feat: support 'prepare' phase before the scene load

6568a29

chore: update CHANGELOG.md

500fc8c

yu-ogi requested review from xnv and ShinobuTakahashi July 26, 2023 06:12

ShinobuTakahashi approved these changes Jul 26, 2023

View reviewed changes

xnv reviewed Jul 26, 2023

View reviewed changes

yu-ogi added 6 commits July 27, 2023 12:12

chore: 根據 -> 根拠

a824ca7

fix: do not modify the loading scene

a84007a

chore: add comments

fca39e8

chore: fix comments

9813ec0

fix: support for async loading scenes

1e96731

fix: modify to wait prepare when popping a scene

4e782df

yu-ogi commented Aug 2, 2023

View reviewed changes

chore: release _currentPrepare when destroyed

af64dd2

xnv reviewed Aug 4, 2023

View reviewed changes

fix: do not call end() if the game has been destroyed

49bd624

xnv reviewed Aug 8, 2023

View reviewed changes

yu-ogi added 3 commits August 9, 2023 15:29

fix: modify method arguments

3acb3e4

chore: _currentPrepare -> _waitingPrepare

32ca1b4

chore: fix comment

89d0fe9

xnv reviewed Aug 14, 2023

View reviewed changes

xnv approved these changes Aug 14, 2023

View reviewed changes

yu-ogi added 2 commits August 14, 2023 15:21

fix: use _pushPostTickTask()

04ff1b3

chore: v3.14.1

3a8eaa1

yu-ogi merged commit ae7c647 into main Aug 15, 2023
12 checks passed

yu-ogi deleted the support-scene-prepare branch August 15, 2023 07:16

yu-ogi mentioned this pull request Aug 15, 2023

chore: fix CHANGELOG.md #458

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: support 'prepare' phase before the scene load #456

feat: support 'prepare' phase before the scene load #456

yu-ogi commented Jul 26, 2023

xnv Jul 26, 2023

yu-ogi Jul 27, 2023

xnv Jul 26, 2023

yu-ogi Jul 27, 2023

xnv Jul 26, 2023

yu-ogi Jul 27, 2023

xnv Jul 26, 2023

yu-ogi Jul 27, 2023

xnv Jul 27, 2023

yu-ogi Jul 27, 2023

yu-ogi Aug 2, 2023

yu-ogi Aug 2, 2023

xnv Aug 4, 2023

yu-ogi Aug 8, 2023

xnv Aug 8, 2023

yu-ogi Aug 9, 2023

xnv Aug 8, 2023

xnv Aug 8, 2023

yu-ogi Aug 9, 2023

xnv Aug 14, 2023 •

edited

Loading

yu-ogi Aug 14, 2023

xnv left a comment

		preserveCurrent?: boolean;
		prepare?: (done: () => void) => void;

feat: support 'prepare' phase before the scene load #456

feat: support 'prepare' phase before the scene load #456

Conversation

yu-ogi commented Jul 26, 2023

このpull requestが解決する内容

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

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

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

xnv Aug 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xnv left a comment

Choose a reason for hiding this comment

xnv Aug 14, 2023 •

edited

Loading