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

If Emission keyword is false, do not output a value #227

Merged
merged 1 commit into from
Mar 18, 2019

Conversation

hiroj
Copy link
Contributor

@hiroj hiroj commented Mar 18, 2019

EMISSIONキーワードがfalseの場合は何も書き込まない

@hiroj hiroj requested a review from yutopp March 18, 2019 10:58
@hiroj hiroj self-assigned this Mar 18, 2019
@hiroj
Copy link
Contributor Author

hiroj commented Mar 18, 2019

Standerdシェーダを使用してエクスポートした時に、EmissionEnable = falseの状態でもEmissionColorが書き込まれてしまい、インポート時にEmissionColorが(0,0,0)でなかった場合はEmissionEnable = trueになってしまうバグの修正

Copy link
Contributor

@yutopp yutopp left a comment

Choose a reason for hiding this comment

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

LGTM with nits.

@@ -142,6 +142,9 @@ static void Export_Occlusion(Material m, TextureExportManager textureManager, gl

static void Export_Emission(Material m, TextureExportManager textureManager, glTFMaterial material)
{
if (m.IsKeywordEnabled("_EMISSION") == false)
return;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return;
{
return;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

nits:

@yutopp yutopp added this to the v0.52.0 milestone Mar 18, 2019
@@ -142,6 +142,9 @@ static void Export_Occlusion(Material m, TextureExportManager textureManager, gl

static void Export_Emission(Material m, TextureExportManager textureManager, glTFMaterial material)
{
if (m.IsKeywordEnabled("_EMISSION") == false)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (m.IsKeywordEnabled("_EMISSION") == false)
if (!m.IsKeywordEnabled("_EMISSION"))

@yutopp
Copy link
Contributor

yutopp commented Mar 18, 2019

自分であればboolの比較ならnotのほうが短くて嬉しいというのと、ifの後で改行するのであればブロックで囲んでおいたほうが事故らなさそうだと思うので、一応メモで書きました。

IDEのフォーマッタ挟まっているので良さそうです。

@yutopp yutopp merged commit 44548b9 into vrm-c:master Mar 18, 2019
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