エンティティの範囲外が描画されている時エラーを投げる処理を追加 #1043

dera- · 2022-03-02T10:17:17Z

概要

下記issueへの対応。Safariのみへの対応は難しいのでコンテンツのテスト環境であるakashic-cli-serveにてエンティティの範囲外が描画されている時エラーを投げる対応を行いました。

Apple端末でnew Sprite時に生成されたエンティティの表示がブラウザ実装により異なる akashic-engine#349

やったこと

akashic-cli-serve上で実行されるコンテンツのPrimarySurfaceやSurfaceにエンティティの範囲外が描画範囲として指定されていた時エラーを投げる処理を差し込んだ

…drawing out of range

xnv · 2022-03-02T10:32:21Z

packages/akashic-cli-serve/src/client/akashic/GameViewManager.ts

@@ -5,8 +5,10 @@ import { ServeGameContent } from "./ServeGameContent";
 import { generateTestbedScriptAsset } from "./TestbedScriptAsset";

 export interface Platform {
+	getPrimarySurface: () => any; // 戻り値は `g.Surface` (コンパイル時に g がないので any で妥協)


any で妥協するより最低限必要なものだけ定義しませんか。(ScriptAsset と違って丸ごと差し替えてるわけでもないですし)　たぶん、このぐらいの話ではないかと思います。

interface RendererLike { drawImage(surface: SurfaceLike, srcX: number, /* 中略 */ destY: number): void; } interface SurfaceLike { width: number; height: number; renderer(): RendererLike; }

オーバーライドする箇所で as any が必要かもしれませんが、その直前までは型つきで書けると思います。

ありがとうございます、確かにここで利用する分だけ型定義したほうがよさそうですね。そのように型を定義しておきます

xnv · 2022-03-02T10:33:18Z

packages/akashic-cli-serve/src/client/akashic/GameViewManager.ts

+					}
+					const renderer = originalRenderer.apply(this);
+					const originalDrawImage = renderer.drawImage;
+					renderer.drawImage = function (surface: any, offsetX: number, offsetY: number, width: number, height: number) {


後から見た時に混乱しそうなので、引数は使わないにしても元関数と同じにしておきたいです。たぶん destX, destY 的な値がありますよね。

destXやdestYは不要だったので省略してましたが、シグニチャも元関数に合わせておくべきでした。。利用しない引数については先頭に_を付けておくことで対応しようと思います

xnv · 2022-03-02T10:49:13Z

packages/akashic-cli-serve/src/client/akashic/GameViewManager.ts

+					const originalDrawImage = renderer.drawImage;
+					renderer.drawImage = function (surface: any, offsetX: number, offsetY: number, width: number, height: number) {
+						if (offsetX < 0 || offsetX + width > surface.width || offsetY < 0 || offsetY + height > surface.height) {
+							throw new Error(`Please draw with following range. x: 0-${surface.width}, y: 0-${surface.height}.`);


「(バグではないが) プラットフォーム固有の描画トラブルを防ぐために akashic serve がエラーにしている」という情報をメッセージに含めたいです。こんな感じですかねえ。

drawImage(): out of bounds. The source rectangle bleeds out the source surface (${surface.width}x${surface.height}). This is not a bug but intentionally prohibited by akashic serve to prevent platform-specific rendering trouble.

ありがとうございます。どの箇所でエラーを出しているのか、バグではないがプラットフォーム固有の描画トラブルを防ぐための対応という説明もいれておくべきでしたね。そのように修正しておきます

xnv · 2022-03-02T10:50:34Z

packages/akashic-cli-serve/src/client/akashic/GameViewManager.ts

+					const originalDrawImage = renderer.drawImage;
+					renderer.drawImage = function (surface: any, offsetX: number, offsetY: number, width: number, height: number) {
+						if (offsetX < 0 || offsetX + width > surface.width || offsetY < 0 || offsetY + height > surface.height) {
+							throw new Error(`Please draw with following range. x: 0-${surface.width}, y: 0-${surface.height}.`);


せっかく issue 契機の対応なので、元の issue へのリンクを // ref. https://〜 とコメントで残しておきませんか。

了解しました。issueへのリンクをコメントとして残しておこうと思います

xnv · 2022-03-02T10:51:18Z

packages/akashic-cli-serve/src/client/akashic/GameViewManager.ts

+							throw new Error(`Please draw with following range. x: 0-${surface.width}, y: 0-${surface.height}.`);
+						}
+						if (width <= 0 || height <= 0) {
+							throw new Error(`Please set width and height to value higher than 0.`);


ここも。こんな感じとか。 drawImage(): nothing to draw. Either width or height is less than or equal to zero. This is not a bug but intentionally prohibited by akashic serve to prevent platform-specific rendering trouble.

ありがとうございます。こちらもそのようにエラー文言を修正しておきます

xnv · 2022-03-02T10:54:53Z

packages/akashic-cli-serve/src/client/akashic/GameViewManager.ts

+		// 描画元の範囲が指定されたwidth,heightを超える値もしくは0以下が与えられた場合Safariでのみ描画されないという問題が発生するので、g.Surface#renderer()でエラーを投げる処理を差し込む
+		// funcにはg.Surfaceを返す関数が渡されることを想定している。本来は型で縛るべきだがコンパイル時に g がないのでFunction型で妥協。


createMeddlingWrappedSurfaceFactory<T extends (...args: any[]) => SurfaceLike>(func: T): T とすれば「妥協」のコメントはなくせそうです。

ありがとうございます。この関数にそのように型を付けおこうと思います

xnv · 2022-03-02T11:00:55Z

packages/akashic-cli-serve/src/client/akashic/GameViewManager.ts

@@ -160,6 +162,38 @@ export class GameViewManager {
 				() => options.g.ExceptionFactory.createAssetLoadError("can not load script: " + assetPath)
 			);
 		};
+		// 描画元の範囲が指定されたwidth,heightを超える値もしくは0以下が与えられた場合Safariでのみ描画されないという問題が発生するので、g.Surface#renderer()でエラーを投げる処理を差し込む


「エラーを投げる処理」が差し込まれてるのは drawImage() かと思います。

詳細な条件はコードを見ればただちに分かることなので、ここでは「一部のエッジケースで Safari のみ描画結果が異なることがあるので」ぐらいでよいかと思います。

またエラーを投げる「意図」もコメントしておきたいです。「ゲーム開発者が開発中に気づけるように、エラーを投げる処理を差し込む」ですよね。

ありがとうございます、なぜエラーを投げるのかの説明が抜け落ちていましたのでこちらの追記も含めて修正しておきます。

xnv · 2022-03-02T11:02:16Z

packages/akashic-cli-serve/src/client/akashic/GameViewManager.ts

+					if (rendererCache) {
+						return rendererCache;
+					}
+					const renderer = originalRenderer.apply(this);


外側の rendererCache を renderer という名前にしてしまえば、ここで const しなくてもそのまま代入するだけでよいのでは。

仰る通りでした、そのように修正しておきます

xnv · 2022-03-02T11:02:38Z

packages/akashic-cli-serve/src/client/akashic/GameViewManager.ts

+				const originalRenderer = surface.renderer;
+				// drawImageメソッドの中で元のdrawImageメソッドを利用する実装のため、rendererをキャッシュしないとrenderer呼び出しの度にバリデーション処理が増えてしまう
+				// 本来なら前回のrendererの内容と比較してdiffがあるかを判定する対応にすべきだが、rendererの内容は不変なので単純にrendererをキャッシュするだけの対応としている
+				let rendererCache: any = null;


別コメントのとおり、ほとんどの any はなくせると思います。

確かに上記でご提案していただいたRendererLikeが利用できそうですね。そのように型付けを修正しようと思います

xnv · 2022-03-02T11:10:20Z

packages/akashic-cli-serve/src/client/akashic/GameViewManager.ts

+				// drawImageメソッドの中で元のdrawImageメソッドを利用する実装のため、rendererをキャッシュしないとrenderer呼び出しの度にバリデーション処理が増えてしまう
+				// 本来なら前回のrendererの内容と比較してdiffがあるかを判定する対応にすべきだが、rendererの内容は不変なので単純にrendererをキャッシュするだけの対応としている


「drawImageメソッドの中で元のdrawImageメソッドを利用する実装のため」に主語がない上にここからちょっと離れた箇所の話だったり、「バリデーション処理」という言葉はここでしか登場しなかったりで、初見だと意図がつかみにくそうです。

前提から全部書くなら

surface.renderer() の戻り値は現実的に固定であり、かつ

surface.renderer() はコンテンツから描画のたびに呼び出されるため、

単純に renderer() をラップして drawImage() を差し替えると描画のたびに差し替えが発生してしまう

ここでの差し替え処理は、「引数のバリデーションを行なって元の関数を呼び出す」ものなので、多重に適用するとバリデーションが多重になってしまう

となると思いますが、簡潔に「surface.renderer() の戻り値は現実的に固定なので、ここでの renderer() 上書きは一度しか適用しない」ぐらいの内容でいいと思います。 if (rendererCache) return rendererCache; のところに書くのでどうでしょうか。

なるほど、3,4つ目については言及してましたが大分説明が抜け落ちていたために分かりにくくなっていたのとどちらかというとディティールの部分なのでそれよりは1,2つ目について言及すべきでしたね。。そのようにコメントを修正しておこうと思います

xnv

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

xnv · 2022-03-04T05:36:39Z

packages/akashic-cli-serve/src/client/akashic/GameViewManager.ts

@@ -4,9 +4,29 @@ import { NullScriptAssetV3 } from "./AssetV3";
 import { ServeGameContent } from "./ServeGameContent";
 import { generateTestbedScriptAsset } from "./TestbedScriptAsset";

+export interface RendererLike {


すいませんこの名前私のコメント由来かと思いますが、 Platform に合わせるなら Renderer が妥当ですね……。

あと、 (Platform も含めて) 可能なら export を外したいです。これらは「元の型定義を参照できない値」を扱うにあたって、"このファイル内で" しょうもない typo などが入り込まないようにするという意味合いが強いためです。「このファイル内の事故防止のための、このファイルが扱うプロパティだけ記述した最低限の定義」が export されるのは違和感があります。

了解しました。「元の型定義を参照できない値」を扱うためだけに型定義しているもの(PlatForm, RendererLike, SurfaceLike)からexportを外す対応をすると同時に、RendererLikeとSurfaceLikeのLikeを外す対応も行います

overwrite getPrimarySurface and createSurface to throw an error when …

0524108

…drawing out of range

dera- requested review from kamakiri01 and xnv March 2, 2022 10:17

xnv requested changes Mar 2, 2022

View reviewed changes

dera- added 2 commits March 3, 2022 09:28

reflect pr comment

2554173

fix detail

189ae9c

xnv approved these changes Mar 4, 2022

View reviewed changes

fix interface in akashic-cli-serve

d8c6187

kamakiri01 approved these changes Mar 4, 2022

View reviewed changes

resolve conflict when merged with master

3d75166

dera- added the enhancement label Mar 8, 2022

dera- merged commit 89576e5 into master Mar 8, 2022

dera- deleted the throw-error-about-drawing-out-of-range branch March 8, 2022 04:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

エンティティの範囲外が描画されている時エラーを投げる処理を追加 #1043

エンティティの範囲外が描画されている時エラーを投げる処理を追加 #1043

dera- commented Mar 2, 2022

xnv Mar 2, 2022

dera- Mar 3, 2022

xnv Mar 2, 2022

dera- Mar 3, 2022

xnv Mar 2, 2022

dera- Mar 3, 2022

xnv Mar 2, 2022

dera- Mar 3, 2022

xnv Mar 2, 2022

dera- Mar 3, 2022

xnv Mar 2, 2022

dera- Mar 3, 2022

xnv Mar 2, 2022

dera- Mar 3, 2022

xnv Mar 2, 2022

dera- Mar 3, 2022

xnv Mar 2, 2022

dera- Mar 3, 2022

xnv Mar 2, 2022

dera- Mar 3, 2022

xnv left a comment

xnv Mar 4, 2022

dera- Mar 4, 2022

		// 描画元の範囲が指定されたwidth,heightを超える値もしくは0以下が与えられた場合Safariでのみ描画されないという問題が発生するので、g.Surface#renderer()でエラーを投げる処理を差し込む
		// funcにはg.Surfaceを返す関数が渡されることを想定している。本来は型で縛るべきだがコンパイル時に g がないのでFunction型で妥協。

		// drawImageメソッドの中で元のdrawImageメソッドを利用する実装のため、rendererをキャッシュしないとrenderer呼び出しの度にバリデーション処理が増えてしまう
		// 本来なら前回のrendererの内容と比較してdiffがあるかを判定する対応にすべきだが、rendererの内容は不変なので単純にrendererをキャッシュするだけの対応としている

エンティティの範囲外が描画されている時エラーを投げる処理を追加 #1043

エンティティの範囲外が描画されている時エラーを投げる処理を追加 #1043

Conversation

dera- commented Mar 2, 2022

概要

やったこと

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

Choose a reason for hiding this comment

xnv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment