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
glTFTexture.source type to int? #1765
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
結構 nullable<T>
にする以外の null チェック挙動変更が見受けられますが、そこが意図した変更なら OK です
} | ||
} | ||
|
||
public static (SubAssetKey, TextureDescriptor) BaseColorTexture(GltfData data, glTFMaterial src) | ||
public static bool TryBaseColorTexture(GltfData data, glTFMaterial src, out (SubAssetKey, TextureDescriptor) value) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
自分としては T?
が書ける現状、 out
キーワードよりも
(SubAssetKey, TextureDescriptor)? BaseColorTexture(GltfData data, glTFMaterial src)
の方がいいのかなとは思うのですがどうでしょう?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
null チェックと tuple 展開を1手でやる方法が無いので。
try かつ tuple やめるやな。
out SubAssetKey key, out TextureDescriptor desc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Item2
がいまいち
@@ -58,8 +58,7 @@ public class glTFTexture | |||
[JsonSchema(Minimum = 0)] | |||
public int sampler; | |||
|
|||
[JsonSchema(Minimum = 0)] | |||
public int source; | |||
public int? source; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nullable
{ | ||
if (string.IsNullOrEmpty(value.Item2.UnityObjectName)) | ||
{ | ||
throw new ArgumentNullException(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
従来なかった null チェックがはさまっていますが、これは?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
もう忘れてしまった。
何らかのエラーが出るのを早期に落とそうとしているぽいが、
忘れたので消そう。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@@ -35,19 +35,20 @@ public static (SubAssetKey, TextureDescriptor) CreateSrgbFromOnlyImage(GltfData | |||
return (texDesc.SubAssetKey, texDesc); | |||
} | |||
|
|||
public static bool TryCreateSrgb(GltfData data, int textureIndex, Vector2 offset, Vector2 scale, out (SubAssetKey, TextureDescriptor) value) | |||
public static bool TryCreateSrgb(GltfData data, int textureIndex, Vector2 offset, Vector2 scale, out SubAssetKey key, out TextureDescriptor desc) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
たしかに out
にするならこっちの方がよさそうですね
fixed #1417