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 chara plus bonus #225

Merged
merged 4 commits into from
Jun 3, 2019

Conversation

kagekiyo7
Copy link
Collaborator

キャラに+ボーナス欄を追加しました。
武器のボーナスもこの形での実装に変更すれば、全武器まとめて付け外しをするみたいな機能の追加もやりやすいのかなと思います。

・問題点
武器のボーナスと同じように最大値がリストの上に来るようにしたかったができなかった
image

@kei-gbf
Copy link
Collaborator

kei-gbf commented Jun 2, 2019

武器のボーナスと同じように最大値がリストの上に来るようにしたかったができなかった

武器の方も明示的なソートはしてませんが、
"key": value の key が "+key" となっていて、恐らく文字列としてソートされてます。

武器ボーナスのつけ外しは、過去の編成データを切り捨てると対応が楽なのですが。
ステータスに既に加算済みで追加されている点が課題になりそうです。

訂正: 文字列としてソートにしても順序が解らないので、
数値順にソートされないようになっているだけかもしれない。


コード整形(空行)

細かい部分だと思われるかも知れませんが、
人によっては開発環境・エディタの設定等でも読みやすさが違ったりするので、
現状では周囲のコードに合わせておくのが無難な部分です。

取り下げる程でもないかなという、微妙な優先度なので、
以下は余談みたいな感じで読み流してください。


コード整形による影響

  • 1ページに表示される情報量が少なくなる
    • 1画面内に空行が多すぎても冗長に感じて返って見通しが悪くなるケースがあります。
      インデントを適切に使うことで段落を表現できて、行数節約にもつながります。
  • プロジェクト全体での統一感
    • 後半部分や他のファイルは詰めたまま。
      とはいえ、全部のコードを整形するのも PR の趣旨とずれてくるので、
      変更要求ではないです。(整形による影響範囲の確認)

理想的には、コーディング規約を決めて自動整形ツールに任せるのが良いのですが、
ここはもう少し環境整備が必要な領域。


後、GitHub の PR に対応した環境では、レビュー時に差分を順番に表示してくれるのですが、
コードの整形は、複数ある場合は コード整形のみでコミットを別けた方がいいです。
コミットログを code cleanup 等として。(1か所のみの場合等は状況次第で)

これはコード整形が頻繁に混ざってると、PRの主な変更点が埋もれてしまう為。
同一PR内でもコミットが別かれてると、選別可能になるのでレビューがやりやすくなります。

一応、インデント調整等は無視する為に diff に空白無視のオプションがあるのですが、
ブラウザ上のレビューでは環境によっては空行無視は未対応だったりします。
(端末上では git diff --ignore-blank-lines で空行変更無視が可能)

This aim to sort charaPlusNumList key as not-number.
As well plusNumList for weapons, (max) entry came the second.
@kei-gbf
Copy link
Collaborator

kei-gbf commented Jun 2, 2019

武器のボーナスと同じように最大値がリストの上に来るようにしたかったができなかった

PR 送りました。


後、コード整形について。具体的な例がないと解りにくいかと思い補足説明。
主に、複数人で共同編集する際に起こる問題です。

  • 後からコードを読んだ他の人が、ここだけコーディングスタイルが揃ってないと思い
    再度修正するかもしれない。
  • (他の編集者が) エディタ等でコードを自動整形する設定にしていると、
    手作業で整形したコードは、編集保存時に自動的に変換される事があります。
    • 取り立てて問題にする程で無いかもしれないけど、エディタ設定の影響は
      現状既に起こっていて例えば *.json ファイルを手作業で編集した際に追加される
      ファイル末尾の改行など。
      エディタ環境の設定の違いの為、付いたり外れたりしてることがある。

Add "+"(plus) to charaPlusNumList key
@kagekiyo7
Copy link
Collaborator Author

kagekiyo7 commented Jun 2, 2019

PRありがとうございます。マージしました。

武器ボーナスのつけ外しは、過去の編成データを切り捨てると対応が楽なのですが。
ステータスに既に加算済みで追加されている点が課題になりそうです。

現状では一々武器を入れ直す必要があるため、個人的には切り捨ててでも実装して欲しい機能ですね

コード整形(空行)

了解です。次からはあまり触らないようにします。

@kei-gbf
Copy link
Collaborator

kei-gbf commented Jun 3, 2019

後は、過去のセーブデータ読み込みのテストですが
データ読み込み時の初期化に漏れがありそうです。

  • 過去の編成データを読み込む時のデータの初期化

再現手順

  • キャラの+ボーナスを適当な値に変える
  • plusBonus のない編成データを読み込む
  • +ボーナスの値が初期化されていない

EXLBや最近追加した他の項目についても、同様のテストが必要かもしれません。

@kei-gbf
Copy link
Collaborator

kei-gbf commented Jun 3, 2019

防御デバフdefenseDebuffでは content.js の handleChangeData で対応してましたが、
キャラや武器等へ追加した項目は個別に対応が必要です。

追加した項目の一覧って作れますか?
恐らく #223 も影響あるので、別PRでまとめて対応しようと思います。

@kagekiyo7
Copy link
Collaborator Author

kagekiyo7 commented Jun 3, 2019

直し方が分からないので対処してくれるというのならありがたいです……!


去年の夏辺りから止まっている計算機とこのPRをデプロイした計算機で項目差分一覧

ジータタブ

ジータさん性別

●ジータさんマスターボーナス

DA、TA、ダメージ上限

●ジータさんリミットボーナス

DA、TA、ダメージ上限UP、奥義ダメージ、属性攻撃、チェンバ、チェンバ上限、

●個別バフ

奥義ダメージUP

●パーティ全体へのバフ等

奥義ダメージUP、奥義ゲージ上昇奥義

召喚石タブ

与ダメ加護、ダメージ上限加護、奥義ダメージUP

キャラタブ

性別、+ボーナス、サポアビ3

●個別バフ

奥義ダメージUP

●EXLB

全て

武器タブ

スキル3


ちなみにまだ報告していなかったのですが、
編成データを読み込む → パラメータを変更する → 同じ編成データを読みこむと、
内部的には編成データの数値が読み込まれているが表示は更新されないという不具合があります。

@kei-gbf
Copy link
Collaborator

kei-gbf commented Jun 3, 2019

同じ編成データを読みこむで内部的には編成データの数値が読み込まれているが表示は更新されないという不具合があるようです。

これは前に直そうとしてよくわからなかった部分かな、他に影響が出たため差し戻した。
確か、編成データ名が同じだと更新されないようです。

@kagekiyo7
Copy link
Collaborator Author

なるほど。構造上直すのは難しいかもしれませんが、一応不具合ということでissues立てておきました。

@kei-gbf
Copy link
Collaborator

kei-gbf commented Jun 3, 2019

データ読み込み時に初期化しようと考えてましたが、
別で進行中の#201 で差分読込を実装するのに競合しそうで、
ここの対応はもう少し考えてからで、後回しになるかもしれないです。

@kei-gbf
Copy link
Collaborator

kei-gbf commented Jun 3, 2019

課題は残りますが、他の項目でも同じ状況なので
対応はまとめて後回しにして先にこのPRを通しますね。

Copy link
Collaborator

@kei-gbf kei-gbf left a comment

Choose a reason for hiding this comment

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

I approve this at the first then fix issue later.

Known Issue:
Loading old save data does not initialize new fields,
which added recently, EXLB etc ... won't fix in this PR.
because another fields also has the same issue.
I will fix in another PR, currently it has conflict with my #201 (WIP)

@kagekiyo7 kagekiyo7 merged commit a7abd5d into MotocalDevelopers:master Jun 3, 2019
@kagekiyo7 kagekiyo7 deleted the Add-chara-plus-bonus branch June 6, 2019 13:25
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.

2 participants