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

全てのタグを追加 #102

Merged
merged 126 commits into from
Dec 6, 2023
Merged

全てのタグを追加 #102

merged 126 commits into from
Dec 6, 2023

Conversation

Shick1112
Copy link
Contributor

初めまして、Shickと申します。
Text2Frameについて、全てのタグが実装されていたら便利だと思い、
実装されていなかったタグを追加してみました。

お時間があればレビューをしてもらえたら嬉しいです。
特に見てほしいのはタグ名やタグに使用する引数、引数名についてです。
※プログラムの書き方は後々修正してもテキストファイルに影響ないですが、タグ名や引数等は影響があるので

以上です。よろしくお願いします。

@yktsr
Copy link
Owner

yktsr commented Oct 4, 2023

プルリクありがとうございます!!
通知が届いておらず気がついていませんでした。レビューさせていただきます!
ありがたいことにとても多くの変更をいただいたので、しばしお時間いただきます。

@HidetoshiKawaguchi
Copy link
Collaborator

HidetoshiKawaguchi commented Oct 4, 2023

@Shick1112
プルリクありがとうございます。仕様・ドキュメント・テストケースは私が担当なので、その観点でザッとレビューしました。

まず、Text2Frameの仕様の癖をほぼ完璧に理解された上で新たなタグの仕様が書かれていることに感服いたしました。私がこれまでに書いた仕様と一貫性がとれているため、仕様の修正は今の段階ではほぼ必要なさそうです。

一方で、テストケースが存在していません。開発者用ドキュメントがないので伝わっていなかったのかなと思うのですが、実は結構ガチガチにテストケースを書いており、それができたものをmasterブランチに取り込む方針をとっております。そのため、テストケースがない現状では、すぐにmasterブランチにはマージできません。何卒ご理解いただけると幸いです。

以下に、masterに取り込むためのタスクを箇条書きします。

  • テストケースの作成と実行(実行は自動)
  • 仕様の最終チェック
  • ドキュメントの調整(タグの番号の並び替えや、Shick様のお名前の追加等)
  • Wikiの更新

以上のタスクはすべて私が実施しようと思います。一応、私がこのプラグイン開発の言い出しっぺであり、仕様の責任を持っておきたいです。テストケースを作りながら、実装いただいた機能の仕様をじっくりと確認させてください。もちろん、バグがないかのチェックも含んでおります。なにぶん、実装いただいたタグが多いので、1ヶ月以上はかかると思います。コツコツとやらせてください。

ザッと見た限りはなかったのですが、じっくり見た上で仕様変更したほうが良さそうな箇所がありましたら、別途ご相談させてください。黙って仕様および実装をするということはしないと、お約束します。

なにか質問・ご意見等ありましたら、何なりとお申し付けください。よろしくお願いいたします。

@Shick1112
Copy link
Contributor Author

@HidetoshiKawaguchi @yktsr
レビュー・対応ありがとうございます!
実装したタグが多いので、時間がすごくかかり大変かと思いますが、何卒よろしくお願いいたします。

バグが出た場合はどうしたらいいでしょうか?
テストケース等は作成していないのですが、こちらでも実際に使用してみてうまく動かなかった箇所が判明した場合は
その都度修正してpush、プルリク等出した方がいいでしょうか?

@HidetoshiKawaguchi
Copy link
Collaborator

@Shick1112
お返事ありがとうございます。
バグについて、軽微なものであれば私が直して置きます。
時間がかかりそう or 仕様を変えるレベルになりそうなものが見つかれば、その時はまたご相談させてください。
よろしくおねがいします。

@HidetoshiKawaguchi
Copy link
Collaborator

HidetoshiKawaguchi commented Oct 7, 2023

ただいま、ドキュメントをwikiに転機しております。その過程で見つけた、おそらく仕様バグやバグだと思われるものを以下にメモ書きしておきます。見つかり次第追記します。

仕様について議論したいこと

  • スキルの増減: forgotは正しくはforget (ツクールの用語に準拠すると)
    • これは軽微であり、文字列の置換のみで対応できるので私が修正する予定です。
  • 吹き出しアイコンの表示 userdefineduser-definedなのではないだろうか
    • 理由があるのであれば良いが、ないのであればツクール準拠のuser-definedとしたい。要確認
  • 移動ルートの設定でのMovePlaySeは、PlaySeと仕様を合わせる?例えば引数が指定されない場合のデフォルト値を設定する
  • 以下の音楽を設定する系のイベントコマンドは、一貫性のためにデフォルト値を設定する
    • 勝利MEの変更
    • 敗北MEの変更
    • 乗り物BGMの変更
    • 移動ルートの設定内でのSEの再生
  • 戦闘処理の<IfEnd><End>と共通でも良いのではないか?
    • 調べてみたら、分岐終了と戦闘処理の終了のjsonのcodeは異なっていた。一旦BattleEndとする。IfEndでは条件分岐のものと紛らわしいので。
  • ショップの処理
    • ショップの処理の引数で必要なのは商品タイプ等は無視されるはずだし。購入のみだけではないか。
    • 2行目以降で商品を指定できるようにするべき。今の仕様では、例えば商品を販売しておらず、売却だけができるショップを作れない仕様になっている。
    • ショップの処理の2行目以降のタグ名が長いので、Merchandise商品にしたい
    • ショップの処理の2行目以降は、「価格タイプ」を排除し、価格を整数値で指定したときのみ価格タイプがspecifyとして扱われ、指定しない場合はstandardにしたほうが使いやすいとおもう。
  • 指定位置の情報取得に、以下の変更を行いたい
    • ChangeGetLocationInfo -> GetLocationInfo: Changeがついているのは、おそらくバグ?
    • MV/MZで挙動を分ける書き方をしない。ドキュメントには、キャラクターを使った指定はMZ限定と記載する
    • Locationの設定を、以下の3種類で分けて、第3引数で指定するものとする。
      • Direct[X座標][Y座標]
      • WithVariables[X座標の変数ID][Y座標の変数ID]
      • Character[イベントのID]
  • 戦闘アニメーションの表示
  • MV/MZの切り替えは、プログラム内で行う。引数でどちらを使うかは指定しないものとする
  • 戦闘行動の強制
    • インデックスの指定で数値が1ずれている。ほかは一致している。バグなのか、ドキュメントへの記載ミスなのかは現段階で不明
  • 戦闘処理の「逃走可」「敗北可」の引数はなくして、タグを入れただけで使えるようにしたほうがユーザフレンドリーだと思う。条件分岐もそのような仕様になっている

バグ

  • 移動ルートの設定の例を動かしたところ、適切な形でインポートされない。

wikiにヘルプ文を移植する際に、(一貫性のために)文章の構造を変えたタグ

  • 移動ルートの設定
  • 画面の色調変更
  • 戦闘の処理
  • ショップの処理

@yktsr
Copy link
Owner

yktsr commented Oct 14, 2023

戦闘処理の<IfEnd>は<End>と共通でも良いのではないか?
調べてみたら、分岐終了と戦闘処理の終了のjsonのcodeは異なっていた。一旦BattleEndとする。IfEndでは条件分岐のものと紛らわしいので。

issue には議論過程が残っていないが、 選択肢の表示( #91)で同じ問題があり、結局全てのブロックタグ(if や choicesなど場合分けが存在するもの)は現状全て<end>で統一することになっている。ブロックの概念があり、<end>は最も内側のタグと組になるようになっているので、codeが異なっていても<end>で可。

Text2Frame-MV/Text2Frame.js

Lines 4845 to 4855 in 7b1d18a

}else if(current_frame.code == WHEN_CANCEL_CODE){
const current_index = block_stack.slice(-1)[0]["index"];
if(current_index != 0){
event_command_list.push(getBlockEnd());
}
block_stack.slice(-1)[0]["index"] += 1;
}else if(current_frame.code == CHOICE_CODE){
block_stack.push({"code": current_frame.code, "event": current_frame, "indent": block_stack.length, "index": 0});
}else if(current_frame.code == IF_CODE){
block_stack.push({"code": current_frame.code, "event": current_frame, "indent": block_stack.length, "index": 0});
}

@yktsr
Copy link
Owner

yktsr commented Oct 14, 2023

差分がわかりにくくなっているので、先行してLinterの修正だけ別のPRで masterへ mergeするのはどうでしょう?
特に異論なければ自分やっちゃいます。

@HidetoshiKawaguchi
Copy link
Collaborator

@yktsr

差分がわかりにくくなっているので、先行してLinterの修正だけ別のPRで masterへ mergeするのはどうでしょう? 特に異論なければ自分やっちゃいます。

特に異論ないので、やってください

@HidetoshiKawaguchi
Copy link
Collaborator

HidetoshiKawaguchi commented Oct 15, 2023

@Shick1112

おつかれさまです。こちらの方で、ドキュメントをwikiへの移植をしながら、仕様についての詳細なレビューを実施しました。
大方のタグは問題がなかったのですが、以下のいくつかのタグについて、恐縮ですが私としては仕様を変更したいと考えております。

  • 音楽の再生に関するタグは、一貫性のためにデフォルト値を設定する。
    • 移動ルートの設定でのSEの再生
    • 勝利MEの変更
    • 敗北MEの変更
    • 乗り物BGMの変更
  • マップのスクロール
  • スキルの増減
  • フキダシアイコンの表示
  • 戦闘の処理
  • ショップの処理
  • 指定位置の情報取得
  • 戦闘アニメーションの表示
  • 戦闘行動の強制

引数の誤字の修正程度のものや、構文を大きく変更したほうが良さそうなものと、実装の修正労力は大小様々です。
私が提案するこれらのタグの新仕様の詳細については、本コメントの下部に記載しています。

ひとまず、Shick1112さんのご意見をお伺いしたいです。労力をかけて仕様を考え実装していただいたのに恐縮です。しかし、Text2Frameをよりユーザーフレンドリーにするには、私としては一部仕様は修正したほうが良いと考えております。仕様変更をし、コードにも手を入れていくという方針を受け入れてくださるかどうか、お聞かせ願えないでしょうか。

また、もし受け入れてくださる場合、その進め方についてもご相談させてください。Shick1112さんのほうで実装をすることは可能でしょうか。やりたい、やりたくないという観点でも構いません。もし出来ない場合は、私か、@yktsr が担当することが可能です。テストケースについては、私が作成を担当します。テストケースを作っているうちに、更に細かな仕様変更等は発生する可能性があることは、予めご承知いただければ幸いです。

よろしくお願いします。

以下、提案する新しい仕様。通し番号は新しいものなので、現実装の番号とは異なることに注意

音楽再生系のタグ

以下のタグでは、音量やピッチにデフォルト値を設定できるようにする。
  • 移動ルートの設定でのSEの再生
  • 勝利MEの変更
  • 敗北MEの変更
  • 乗り物BGMの変更

例えば、BGMの再生は既にそのような仕様になっているので、それと同様のことを行う。

(30) スキルの増減

操作の指定の`forgot`を`forget`に変更する。

(39) マップのスクロール

完了までウェイトを、MVという指定をなくして、省略可能にする。

マップのスクロールの新仕様

「マップのスクロール」は以下のいずれかの記法で組み込むことができます。

  • <ScrollMap: 方向, 距離, 速度, 完了までウェイト>
  • <マップのスクロール: 方向, 距離, 速度, 完了までウェイト>

向きリスト

  • 下: down, 2,
  • 左: left, 4,
  • 右: right, 6,
  • 上: down, 8,

速度

  • 1/8倍速: x8slower, 1, 1/8倍速
  • 1/4倍速: x4slower, 2, 1/4倍速
  • 1/2倍速: x2slower, 3, 1/2倍速
  • 標準速: normal, 4, 標準速
  • 2倍速: x2faster, 5, 2倍速
  • 4倍速: x4faster, 6, 4倍速

完了までウェイトリスト

  • チェックオン: true, 1, オン, ON
  • チェックオフ: false, 0, オフ, OFF

完了までウェイトは省略可能です。その場合は、チェックオフとなります。
また、完了までウェイトをオンにできるのは、RPGツクールMZ限定です。

例1: 下方向の距離100、標準速でマップをスクロール 完了までウェイトしない

  • <ScrollMap: down, 100, normal>
  • <マップのスクロール: 下, 100, 標準速>
  • <ScrollMap: down, 100, normal, false>
  • <マップのスクロール: 下, 100, 標準速, オフ>

例2: 右方向の距離50、1/2倍速でマップをスクロール 完了までウェイト

  • <ScrollMap: right, 50, x2slower, オン>
  • <マップのスクロール: 右, 50, 1/2倍速, true>

(46) フキダシアイコンの表示

`userdefined`を`user-defined`に変更する。 ちょっと悩みましたが、一応ツクール準拠にするためにこちらに合わせたいです。

(70) 戦闘の処理

以下の3点を変更する

  • 逃走可敗北可の引数を削除
  • 敵グループの指定方法
  • <IfEnd>タグを<End>タグに変更。
    以下に、新しい仕様の詳細を示します。

戦闘の処理の新仕様

「戦闘の処理」は以下の記法で組み込むことができます。

<BattleProcessing: 敵グループ>
<IfWin>
勝利した時の処理
<IfEscape>
逃走したときの処理
<IfLose>
敗北したときの処理
<End>

BattleProcessing戦闘の処理でも代替できます。
また、IfWin勝ったときIfEscape逃げたときIfLose負けたときEnd分岐終了で代替できます。

敵グループは、以下の3種類の指定方法があります。

  • 直接指定: 敵グループID
  • 変数の指定: Variables[変数ID]または変数[変数ID]
  • ランダムエンカウント: Random, ランダム

<IfWin>タグ、<IfEscape>タグ、<IfLose>タグは省略可能です。
また、これら3つをすべて省略したときに限り、<End>タグも省略可能です。

以下に具体例を示します。

例1: 敵グループID1とエンカウント 逃走不可 敗北不可

  • <BattleProcessing: 1>
  • <戦闘の処理: 1>

例2: 変数ID5の敵グループとエンカウント 逃走可 敗北可

<BattleProcessing: Variables[5]>
<IfWin>
勝った!
<IfEscape>
逃げた!
<IfLose>
負けた!
<End>

または

<戦闘の処理: 変数の指定, Variables[5]>
<勝ったとき>
勝った!
<逃げたとき>
逃げた!
<負けたとき>
負けた!
<分岐終了>

例3: ランダムな敵グループとエンカウント 逃走不可 敗北不可

  • <BattleProcessing: Random>
  • <戦闘の処理: ランダム>

(71) ショップの処理

ショップの処理では、かなりいろいろ仕様を変更する必要があるので、差分は示さずに、以下に新しい仕様をそのまま示します。

ショップの処理の新仕様

「ショップの処理」は以下のいずれかの記法で組み込むことができます。

<ShopProcessing: 購入のみ>
<Merchandise: 商品タイプ, 商品ID, 価格>
・・・以下任意の数の商品を示すタグ

ShopProcessingは、ショップの処理で、Merchandiseは、商品で代替できます。
Merchanciseタグは、販売するアイテム・装備品を示すタグであり、任意の数を続けて指定できます。なしということも可能です。

購入のみリスト

  • チェックオン: true, 1, オン, ON
  • チェックオフ: false, 0, オフ, OFF

商品タイプリスト

  • アイテム: item, 0, アイテム
  • 武器: weapon, 1, 武器
  • 防具: armor, 2, 防具

価格

  • 標準: standard, 標準
  • 指定: 整数値をそのまま指定

例1: アイテムID1を標準価格に設定 購入のみではない

<ShopProcessing: false>
<Merchandise: Item, 1, standard>

または

<ショップの処理: オフ>
<商品: アイテム, 1, 標準>

例2: 複数の商品を設定 購入のみ
アイテムID1 標準価格
武器ID4 価格500
防具ID6 価格1200

<ShopProcessing: true>
<Merchandise: weapon, 4, 500>
<Merchandise: armor, 6, 1200>

または

<ショップの処理: オン>
<商品: 武器, 4, 500>
<商品: 防具, 6, 1200>

(92) 指定位置の情報取得

以下に、新しい仕様をそのまま示します。

指定位置の情報取得の新仕様

「指定位置の情報取得」は以下のいずれかの記法で組み込むことができます。

  • <GetLocationInfo: 変数ID, 情報タイプ, 位置>
  • <指定位置の情報取得: 変数ID, 情報タイプ, 位置>

情報タイプリスト

  • 地形タグ: terraintag, 0, 地形タグ
  • イベントid: eventid, 1, イベントid
  • レイヤー1: layer1, 2, レイヤー1
  • レイヤー2: layer2, 3, レイヤー2
  • レイヤー3: layer3, 4, レイヤー3
  • レイヤー4: layer4, 5, レイヤー4
  • リージョンid: regionid, 6, リージョンid

位置は、以下の記法で組み込みます。

  • 直接指定: 以下のいずれか
    • Direct[X座標][Y座標]
    • 直接指定[X座標][Y座標]
  • 変数で指定: 以下のいずれか
    • WithVariables[X座標を指定する変数のID][Y座標を指定する変数のID]
    • 変数で指定[X座標を指定する変数のID][Y座標を指定する変数のID]
  • キャラクターで指定: 以下のいずれか
    • Character[イベントID]
    • キャラクター[イベントID]

キャラクターで指定する場合のイベントIDリスト

  • プレイヤー: player, -1, プレイヤー
  • このイベント: thisevent, 0, このイベント
  • EV00n: n

なお、キャラクターで指定するのはツクールMZのみの機能です。

例1: 変数ID1に、現在のマップのX座標10,Y座標20の地形タグの値を格納する

  • <GetLocationInfo: 1, terraintag, Direct[10][20]>
  • <指定位置の情報取得: 1, 地形タグ, 直接指定[10][20]>

例2: 変数ID2に、現在のマップのX座標を変数4で、Y座標を5で指定しレイヤー1のタイルIDを格納する。

  • <GetLocationInfo: 1, terraintag, WithVariables[4][5]>
  • <指定位置の情報取得: 1, 地形タグ, 変数で指定[4][5]>

例3: 変数ID3に、このイベントのリージョンIDの値を格納する

  • <GetLocationInfo: 3, regionid, Character[0]>
  • <指定位置の情報取得: 3, リージョンid, キャラクター[このイベント]>

(100) 戦闘アニメーションの表示

- MV/MZの切り替えは、プログラム内で行う。引数でどちらを使うかは指定しないものとする

(101) 戦闘行動の強制

インデックスの指定で数値が1ずれているので、それを他のタグと同様に合わせる

@Shick1112
Copy link
Contributor Author

@HidetoshiKawaguchi
仕様レビューありがとうございます!

■仕様変更について
HidetoshiKawaguchiさんが仕様変更を推奨した箇所についてはもちろん修正したほうがいいと思います。
※私が実装した箇所については仕様が荒かったり、統一されていない箇所があると思うので。

■実装について
私が対応したいと思います。
提案していただいた仕様について不明な点があった場合は相談させてください。

■通し番号について
通し番号についても修正したほうがいいでしょうか?
修正箇所は「対応しているコマンド~」と各タグの使い方の部分の番号と順番になるかと思います。

以上です。よろしくお願いいたします。

@HidetoshiKawaguchi
Copy link
Collaborator

@Shick1112
お返事ならびに、仕様変更、さらに実装のお引き受けをいただきありがとうございます!大変嬉しいです。

最初に、今後の流れと実装手順についてご説明させていただきます。大きな流れをまず説明すると、以下の通りとなります。

  1. Shick1112さんのリポジトリにイシューをいくつか発行し、テストケースをfeature/add_all_tagブランチか、イシューに紐づけた新たなブランチにプッシュ(私が担当)。これらのイシューは、仕様の変更だったりテストケースの追加だったりします。
  2. 1.で作成されたイシューを解決する実装や修正を行い(Shick1112さんが担当)、feature/add_all_tagブランチにマージ。イシュー毎にfeature/add_all_tagブランチにマージします。一応、イシュー毎にプルリクを発行したいです(プルリクのレビューは、私と@yktsr が担当)。
  3. 1.で発行されたイシューがすべて解決されていれば、本プルリクを受領し、オリジナルのText2Frameリポジトリにマージ

となります。

以上の流れに則ると、まず私がイシューを発行する必要があるので、リポジトリの設定を変えてイシューを発行できるようにしていただけないでしょうか?おそらく、Shick1112さんには画面上のメニューの一番右に「Settings」という項目があり、ここから設定できるかと思います。

イシューを発行できるようになった後に、私のほうでイシューをいくつか発行します。また、それと同時に、それらの仕様を満たしているか確認するためのテストケースを feature/add_all_tagブランチか新たなブランチにプッシュさせていただきます。(Text2Frameは、TDDに則って開発しております)

いろいろと書きましたが、ひとまずの依頼事項をまとめると以下のようになります。

  • まず、イシューを発行できるようにリポジトリの設定を変更してください
  • 次に、私がイシューを発行したりテストケースを書いてfeature/add_all_tagブランチか新たなにブッシュするので、しばらくお待ちください。これも1週間〜2週間程度かかる見込みです。終わった時か、2週間以上かかる場合はお声がけします。またその際、実装手順等は改めてお知らせします。

通し番号についても修正したほうがいいでしょうか?

これは修正しなくて大丈夫です。実装が終わったあとに、私がText2Frame内のドキュメントに手を入れます。そのときに通し番号も合わせます。

ちょっと込み入った話になっており恐縮ですが、よろしくお願いします。
わからないことは、お手数ですがお問い合わせいただけると幸いです。

補足: TDDについて

Text2Frameは、テスト駆動開発(TDD)に則って開発しております。README.mdの下部に、テストの実行方法が書かれております。

npm run test

実行には、nodo.jsのmochaというテストフレームワークを使っているので、必要に応じてインストールしてください。なお、テストのコードは、test/test_json_eq.jsonにあります。

masterブランチは、原則的に常にこのテストをパスしている状態となっております。

新たな機能の仕様を追加するたびに、テストケースを追加しております。現状本プルリクの新機能にはまだテストケースが存在しないので、それはこれから私が作っていきます。

Shick1112さんが実装されたときは、このnpm run testを実行し、新たに実装している機能のテストがパスされているかを確認していただけると進行がスムーズになるかと思います。(実行が難しくても、こちらで確かめます)

@yktsr
Copy link
Owner

yktsr commented Oct 15, 2023

@Shick1112
私からも一つお願いが。
ファイル全体に何らかのコードフォーマッターがかかっているのですが、それのルールがわかりませんでした。参照先か、または可能であれば、下記eslintファイルの形式でいただけると、コマンドライン or Github Actions で検証できるようになります。

{
"env": {
"browser": true,
"commonjs": true,
"es6": true
},
"extends": "eslint:recommended",
"globals": {
"Atomics": "readonly",
"SharedArrayBuffer": "readonly"
},
"parserOptions": {
"ecmaVersion": 2018
},
"rules": {
}
}

$ npm run lint

@Shick1112
Copy link
Contributor Author

@HidetoshiKawaguchi
丁寧な説明ありがとうございます。
一先ずissuesのチェックをオンにしissuesタブが出てくるようになったので、有効化されたと思います。
issuesが有効化されていなかったり、何か別の設定が必要だった場合はお手数ですが再度ご連絡いただけると助かります!

@yktsr
大変申し訳ありません。
私のVSCodeではprettierというコードフォーマッターを使用しており、
私がText2Frameを修正したタイミングで既存のコードフォーマットを崩している箇所がありました(プロパティのダブルクォーテーション等)

eslintとtestを実行してみたところ、エラーが出力されずうまくいっているようでしたが、
既存のコードフォーマットを崩した箇所は元のコードフォーマットに戻した方がいいでしょうか?
※実装の際は再度lint、testは行う予定です。
・npm run lint
 ⇒エラーが出力されない
・npm run test
 ⇒「mocha --inline-diffs test」が実行され、Basic、Crit等の57項目のテストが全てチェックOK

以上です。よろしくお願いいたします。

@HidetoshiKawaguchi
Copy link
Collaborator

@Shick1112 ありがとうございます!イシューを発行できそうなことを確認いたしました。明日より作業に移りますので、しばらくお待ちください。

@HidetoshiKawaguchi
Copy link
Collaborator

@Shick1112 すみません、イシューは発行できたのですが、追加で以下の権限を私と @yktsr に付与していただけないでしょうか?

  • イシューの各種設定を変更する権限(Assigneesやlabelsを変更する権限)
  • リモートブランチにPUSHする権限。
    Issueに紐付いたブランチをPUSHしたいです。テストケースをお送りするためです。

五月雨で申し訳ないのですが、よろしくお願いいたします。

@Shick1112
Copy link
Contributor Author

@HidetoshiKawaguchi
お二方にManage accessのinviteを送りました!
一旦これで各種操作が出来るかどうか試してもらいますでしょうか?

以上です。よろしくお願いいたします。

@HidetoshiKawaguchi
Copy link
Collaborator

@Shick1112 迅速なご対応ありがとうございます!先程、そちらのリポジトリにてイシューの設定を変え、ブランチもプッシュしました。それぞれのタグの実装については、そちらのリポジトリのイシューやプルリクのコメント欄にて議論しましょう!

@yktsr
Copy link
Owner

yktsr commented Oct 18, 2023

eslintとtestを実行してみたところ、エラーが出力されずうまくいっているようでしたが、
既存のコードフォーマットを崩した箇所は元のコードフォーマットに戻した方がいいでしょうか?

ありがとうございます。一旦自分の方で引き取らせてください。
Google JavaScript Style に合わせて少し調整します。

@Shick1112
Copy link
Contributor Author

@yktsr
お手数おかけして申し訳ございません。
よろしくお願いいたします。

@HidetoshiKawaguchi
Copy link
Collaborator

@yktsr
本プルリク、すべてのタスクが完了しました。最後に確認していただき、よければ dev220 ブランチにマージしてください。

@yktsr
Copy link
Owner

yktsr commented Dec 3, 2023

実機での動作確認って取れてます?

@HidetoshiKawaguchi
Copy link
Collaborator

@yktsr とれてます。wikiのmessage.txt(全機能確認用テキスト)をすべてのタグ対応版つくって、ツクールMV/MZの両方で動かして問題なかったです。

@yktsr
Copy link
Owner

yktsr commented Dec 4, 2023

せっかく全てのタグに対応したので、正面玄関のREADME.mdも説明の順序を少し変えたほうがいいかな、とも思いましたがどうでしょう。あのREADMEは、ver1.0系の、機能が少なかった頃に書かれたもので、今の多機能になった現状では少し読みにくいかもしれません。
特に、

  • 「その他の機能」の記載粒度が揃ってないのが気になってます。コモンイベント書き出しとか、MZの細かい記述はwikiにして、タグの早見表か、サンプルの文例を貼ったほうがよくないでしょうか。
  • トップのgif動画も少し古くて、右クリックからイベント実行、の例に直したほうがbetterでしょうか。あのgifがわかりやすいかは正直自分ではわからないので。

ただ、文章を簡単にイベントに取り込む、という中心部分は強力でわかりやすい部分なので、そこはブレない記述が良いと思います。(つまり、前半はそのままに)

@yktsr
Copy link
Owner

yktsr commented Dec 4, 2023

本文のレビューはもう少し時間をください。

@HidetoshiKawaguchi
Copy link
Collaborator

@yktsr
たしかにREADMEもこれを機に一新して良さそうですね。そうしましょう。
#95
を、ヘルプドキュメントの完全日本語から拡張して、README.mdの更新も入れることにします。

一旦私が更新してプルリクだすので、その際にチェックしてみてください。

ただ、文章を簡単にイベントに取り込む、という中心部分は強力でわかりやすい部分なので、そこはブレない記述が良いと思います。(つまり、前半はそのままに)

これはそのとおりだと思ってて、前半は変えないでいいと思います。Text2Frameが多くの人に受け入れられたことの80%くらいは、基本機能のメッセージをきれいに取り込めることだと思っています。なので、この基本機能は強調している構成は変えないでいきましょう。

トップのgif動画も少し古くて、右クリックからイベント実行、の例に直したほうがbetterでしょうか。あのgifがわかりやすいかは正直自分ではわからないので。

これもそのとおりで、右クリックからのイベント実行にします。wikiのほうはその手順になっているので、それに準拠しましょう。gifじゃなくて、Youtubeからの埋め込み動画が良さそうです。動画撮影も含めて、ちょっと考えてみます。

「その他の機能」の記載粒度が揃ってないのが気になってます。コモンイベント書き出しとか、MZの細かい記述はwikiにして、タグの早見表か、サンプルの文例を貼ったほうがよくないでしょうか。

機能の概要くらいは合ったほうがいいと思います。今のところ、個人的には以下のような変更が良いと思っています。

  • 追加機能のタグをその他の機能よりも前に持ってくる。
  • 早見表をもっとコンパクトに。本当によく使いそうなものだけに絞ったり、複数の表記法をやめる等

@yktsr
Copy link
Owner

yktsr commented Dec 5, 2023

YouTubeはあってもいいですが、再生ボタンを押す手間があるので、一番トップはgifがいいと思います。

@HidetoshiKawaguchi
Copy link
Collaborator

再生とか停止を押すほうが便利だとは思ったのですが、まぁ詳しい動かし方は文章でもwikiでも説明しているし、ここでの動画は動かし方のイメージを伝えるものと割り切ったということでアニメーションgifのほうが良さそうか

@HidetoshiKawaguchi
Copy link
Collaborator

@yktsr
すまんが、コードレビューしてもらってマージ後、以下の2つのバグ対応お願いできないですかね?

テストケースはイシューに紐付いているブランチにいれてます

Text2Frame.js Outdated Show resolved Hide resolved
Text2Frame.js Show resolved Hide resolved
Text2Frame.js Outdated Show resolved Hide resolved
Text2Frame.js Outdated Show resolved Hide resolved
@yktsr yktsr self-requested a review December 6, 2023 09:29
Copy link
Owner

@yktsr yktsr left a comment

Choose a reason for hiding this comment

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

@Shick1112 @HidetoshiKawaguchi
長期間に渡っての作業・実装・レビューありがとうございました。
Approveし、2.2.0のリリース候補ブランチへ投入したいと思います。
リリースまで、もう少し作業がありますが、引き継ぎ協力をお願いします。

@yktsr yktsr merged commit 266f8bf into yktsr:dev-220 Dec 6, 2023
1 check passed
@HidetoshiKawaguchi
Copy link
Collaborator

@yktsr レビューありがとうございます。ひとまず2.2.0に合流でき、ホッとしております。

@Shick1112 ついにyktsr のリポジトリにマージできました。プルリクのご提案から細かい仕様の実装にお付き合いいただきありがとうございます。

2.2.0のリリース(masterへのマージ)は残りの3つを実施した後に行います。これらは私と @yktsr が対応いたします。

引き続き、よろしくお願いいたします。

@yktsr yktsr mentioned this pull request Dec 6, 2023
@yktsr
Copy link
Owner

yktsr commented Dec 10, 2023

先ほど、修正をmasterに投入し、2.2.0をタグ付け・採番しました。
多くの機能追加、テスト、ドキュメントの整備などなど、ほぼ2ヶ月に渡って作業いただいて、ここまでこれたのもひとえにお二人の作業のおかげで、感謝の念に絶えません。
年内にMergeできたのも驚きの速さだったと思います。
改めて作業ありがとうございました!

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.

None yet

3 participants