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

UV rotation UI improvement #79

Merged
merged 3 commits into from
Jun 9, 2019
Merged

UV rotation UI improvement #79

merged 3 commits into from
Jun 9, 2019

Conversation

iCyP
Copy link
Contributor

@iCyP iCyP commented Jun 8, 2019

about #13 (comment)
UI側で値をEnum単位に応じて変換してからシェーダに渡すようにしてみました。

既知の問題:単位を変換しても、UIの値がそれに追従しない。(360度/secー(回転に変更)->360回転/sec、になる。

@Santarh
Copy link
Owner

Santarh commented Jun 8, 2019

PullRequest のほうありがとうございます。
ユーザインタフェースとして親切であり、必要な機能だと思います!
したがってこのまま Merge させていただきたいところなのですが、保守性の観点から以下の点が修正されることが望ましいです。

  • コードのスタイルをあわせる
    • 関数名は UpperCamelCase 、private field は underscore prefix 付き _lowerCamelCase
    • 細かいところでの Spacing や改行を行う
  • git による編集履歴管理が行われているため、不必要となったコードはコメントアウトではなく削除
  • enum を網羅する形での分岐は if else if ではなく switch で行う
  • enum のデフォルト値は 2 ではなく 0 に定義する

また

既知の問題

についても解決されなければ、ユーザインタフェース上、ユーザに混乱を与えるので修正していただければと思います。

@@ -317,7 +320,11 @@ private void Draw(MaterialEditor materialEditor, Material[] materials)
materialEditor.TexturePropertySingleLine(new GUIContent("Mask", "Auto Animation Mask Texture (R)"), _uvAnimMaskTexture);
materialEditor.ShaderProperty(_uvAnimScrollX, "Scroll X (per second)");
materialEditor.ShaderProperty(_uvAnimScrollY, "Scroll Y (per second)");
materialEditor.ShaderProperty(_uvAnimRotation, "Rotation (per second)");
uvRotationUnit = (MToon.RotationUnit)EditorGUILayout.EnumPopup("Rotation Unit", uvRotationUnit);
Copy link
Owner

Choose a reason for hiding this comment

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

int から enum のキャストも dynamic なキャストになるのは危険なので、下の方に定義してある PopUpEnum<T> を使用していただければと思います。

@iCyP
Copy link
Contributor Author

iCyP commented Jun 8, 2019

  • コードのスタイルをあわせる
    1. 関数名は UpperCamelCase 、private field は underscore prefix 付き _lowerCamelCase
    2. 細かいところでの Spacing や改行を行う
  1. 直しました。private static は既存コードに合わせた形です。
  2. 雰囲気でできる範囲は行いました。
  • git による編集履歴管理が行われているため、不必要となったコードはコメントアウトではなく削除

debug用(シェーダに渡される値の確認用)に残していましたが、削除しました。

  • enum を網羅する形での分岐は if else if ではなく switch で行う
  • enum のデフォルト値は 2 ではなく 0 に定義する

行いました。

  • 既知の問題

解決しました。

  • PopupEnumの使用

MaterialPropertyではないので使用できませんでした。そのため、switch文で実装しました。

また、先のプルリクエストより動作の変更があり、角度単位の変更をインスペクタ共通で変更されるようにしました。単位系が混在するとややこしいので、一回変更すれば他も変更された方が便利かなと考えた次第です。

@Santarh
Copy link
Owner

Santarh commented Jun 8, 2019

修正のほう、ありがとうございます!マージさせていただきます。
今後、他の修正で Reformat をかけたときに一部コードが修正されると思いますがご了承ください。

また、先のプルリクエストより動作の変更があり、角度単位の変更をインスペクタ共通で変更されるようにしました。

良いとおもいます!

}
case MToon.RotationUnit.Radians:
{
return val / 2.0f;
Copy link
Owner

@Santarh Santarh Jun 8, 2019

Choose a reason for hiding this comment

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

すみません、最後にひとつだけ……こちらは 2.0f ではなくて 2.0f * Math.PI ではないでしょうか?

@iCyP
Copy link
Contributor Author

iCyP commented Jun 8, 2019

お手数をおかけしまして申し訳ありません。ご指摘の通りです。修正いたしました。
休日深夜に大変お世話かけまして、ありがとうございます。

私自身書いたコードには良くも悪くも無頓着であるので、関数、変数、ロジック、コードフォーマット等々ご自由に変更していただいて構いません。

@Santarh
Copy link
Owner

Santarh commented Jun 9, 2019

修正のほうありがとうございます。
いえいえ、PullRequest していただけるだけでもとてもありがたいので、今後もなにかあればぜひよろしくお願いいたします。
マージします!

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