Skip to content

be able to specify maximum count of multi-tap #419

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

Merged
merged 7 commits into from
Jan 25, 2023
Merged

be able to specify maximum count of multi-tap #419

merged 7 commits into from
Jan 25, 2023

Conversation

dera-
Copy link
Contributor

@dera- dera- commented Dec 2, 2022

このpull requestが解決する内容

game.json の "maxPoints" で最大同時タップ数を制限できるように

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

  • なし

@dera- dera- requested review from xnv and ShinobuTakahashi December 2, 2022 07:55
package.json Outdated
@@ -4,7 +4,7 @@
"description": "The core library of Akashic Engine",
"main": "index.js",
"dependencies": {
"@akashic/game-configuration": "~1.6.0",
"@akashic/game-configuration": "file:../game-configuration/akashic-game-configuration-1.6.0.tgz",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

こちらローカルで動作確認するために一時的にこのような依存にしています。
akashic-games/game-configuration#33 がマージされましたら修正します

@@ -48,16 +53,23 @@ export interface PointEventResolverParameterObject {
export class PointEventResolver {
_sourceResolver: PointSourceResolver;
_playerId: string;
_maxPoints: number | null;
private _currentPoints: number = 0;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

_pointEventMapのキー数を数えることも考えましたが、point~()メソッドは頻繁に呼ばれるのでそこで何度もObject.keysを呼び出すのは処理の無駄かと思いましたので、この変数を用意しました

pointDown(e: PlatformPointEvent): pl.PointDownEvent {
pointDown(e: PlatformPointEvent): pl.PointDownEvent | null {
if (this._maxPoints != null && this._currentPoints >= this._maxPoints) {
return null;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

他のメソッドに合わせて、タップの上限数を超えた時はnullを返すようにしてます

@@ -84,6 +96,7 @@ export class PointEventResolver {
point.y, // 5: Y座標
targetId // 6?: エンティティID
];
this._currentPoints++;
Copy link
Member

Choose a reason for hiding this comment

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

これ前後の行で ret を組み立てている途中に割り込む形になっているので動かしたいです。 _pointEventMap の状態と整合的でなければならない値なので、 _pointEventMap への代入の直後にやりませんか。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

了解しました、そのように修正します。
同時にpointUp()でのthis._currentPoints--_pointEventMapからオブジェクトを削除した直後に行おうと思います。

Comment on lines 54 to 61
expect(e!.length).toBe(7);
expect(e![0]).toBe(pl.EventCode.PointDown); // 0: イベントコード
expect(e![1]).toBe(EventPriority.Joined); // 1: 優先度
expect(e![2]).toBe("dummyPlayerId"); // 2: プレイヤーID
expect(e![3]).toBe(0); // 3: ポインターID
expect(e![4]).toBe(10); // 4: X座標
expect(e![5]).toBe(20); // 5: Y座標
expect(e![6]).toBeUndefined(); // 6?: エンティティID
Copy link
Member

Choose a reason for hiding this comment

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

(ラッパー関数を定義しなくちゃいけないあたり) 一長一短ですが、こういう手もあるようです: DefinitelyTyped/DefinitelyTyped#41179 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ありがとうございます。expect_toBeDefined()に相当する関数をhelperディレクトリ以下に定義して利用してみようと思います

@@ -31,6 +31,11 @@ export interface PointEventResolverParameterObject {
* プレイヤーID
*/
playerId: string;

/**
* 同時にタップ可能な上限
Copy link
Member

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.

了解しました、こちら修正します

@@ -208,4 +211,37 @@ describe("PointEventResolver", () => {
})
).toBeNull();
});

it("can not multi-tap more than maxPoints", () => {
Copy link
Member

Choose a reason for hiding this comment

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

余談: multi-tap は英語としてポピュラーな表現ではなさそうです。 multi touch がよいかもしれません。

一箇所だけ Android のページで https://developer.android.com/training/gestures/multi?hl=ja マルチタップと表記がありますが、英語版ページだと multi-touch になっています。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

なるほど、、英語での表現だとmulti-touchの方がよく使われる感じなんですね。ありがとうございます、そのように修正しておきます

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.

@@ -0,0 +1,4 @@
export function expectToBeDefined<T>(arg: T): asserts arg is NonNullable<T> {
expect(arg).toBeDefined();
// if (arg == null) throw new Error("arg is null");
Copy link
Member

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.

了解しました、こちら削除します

@@ -11,8 +11,8 @@ import {
PointUpEvent,
PointMoveEvent
} from "..";
import type { Runtime } from "./helpers";
import { customMatchers, EntityStateFlags, Game, Renderer, skeletonRuntime } from "./helpers";
import type { Runtime} from "./helpers";
Copy link
Member

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.

あ、、こちらも修正します

@dera- dera- merged commit d44b1fc into main Jan 25, 2023
@dera- dera- deleted the add-max-points branch January 25, 2023 11:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants