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
runtime に BlendShapeProxy.m_merger を再初期化する #1720
runtime に BlendShapeProxy.m_merger を再初期化する #1720
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.
Breaking change が多そうなので意図しているのかどうかで気になりポイントです
@@ -44,9 +53,10 @@ private void Start() | |||
/// <param name="value"></param> | |||
public void ImmediatelySetValue(BlendShapeKey key, float value) | |||
{ | |||
if (m_merger != null) | |||
var merger = Merger; | |||
if (merger != 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.
挙動が変わってしまっている点が気になります。
Start()
するより前の段階で ImmediatelySetValue()
を呼び出すとその状態で初期化され SetValue が走ってしまう、ということになってしまっていると思います。
{ | ||
m_merger = new BlendShapeMerger(BlendShapeAvatar.Clips, transform); | ||
} | ||
m_merger.RestoreMaterialInitialValues(BlendShapeAvatar.Clips); |
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.
これが Destroy されたあとにアクセスされることを考えると
正確にはここで bool isDisposed
という変数に対して true
を代入しておいて
Merger
の getter
では if (isDisposed) return null;
というのを書いた方がよさそうです。
Clear をやめて Reinitialize に変えます |
} | ||
} | ||
} | ||
|
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.
変化が少なくなるように、 Clear
をやめて Restart
に変更しました。
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.
Reinitialize()
などの方が命名としてはしっくりくるが、どうだろう。
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.
と内容がほぼ同じ。ということは Start を呼ぶ関数にしよう 🤔
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.
よさそう。ただ Restart
という名前だとなんだか気軽に呼んでいいような感じにも読める(別に気軽に呼んでもなんも起きないけど)
} | ||
} | ||
} | ||
|
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.
Reinitialize()
などの方が命名としてはしっくりくるが、どうだろう。
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
fixed #1710