{"action":"created","issue":{"url":"https://api.github.com/repos/urho3d/Urho3D/issues/2252","repository_url":"https://api.github.com/repos/urho3d/Urho3D","labels_url":"https://api.github.com/repos/urho3d/Urho3D/issues/2252/labels{/name}","comments_url":"https://api.github.com/repos/urho3d/Urho3D/issues/2252/comments","events_url":"https://api.github.com/repos/urho3d/Urho3D/issues/2252/events","html_url":"https://github.com/urho3d/Urho3D/issues/2252","id":294145044,"number":2252,"title":"Specular is multiplied by brightness twice.","user":{"login":"Bananaft","id":11929430,"avatar_url":"https://avatars1.githubusercontent.com/u/11929430?v=4","gravatar_id":"","url":"https://api.github.com/users/Bananaft","html_url":"https://github.com/Bananaft","followers_url":"https://api.github.com/users/Bananaft/followers","following_url":"https://api.github.com/users/Bananaft/following{/other_user}","gists_url":"https://api.github.com/users/Bananaft/gists{/gist_id}","starred_url":"https://api.github.com/users/Bananaft/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/Bananaft/subscriptions","organizations_url":"https://api.github.com/users/Bananaft/orgs","repos_url":"https://api.github.com/users/Bananaft/repos","events_url":"https://api.github.com/users/Bananaft/events{/privacy}","received_events_url":"https://api.github.com/users/Bananaft/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":3,"created_at":"2018-02-03T21:05:26Z","updated_at":"2018-02-05T13:52:48Z","closed_at":null,"author_association":"NONE","body":"When Batch.cpp sets shader parameters:\r\n\r\n```\r\ngraphics->SetShaderParameter(PSP_LIGHTCOLOR, Color(light->GetEffectiveColor().Abs(),\r\n                light->GetEffectiveSpecularIntensity()) * fade);\r\n```\r\nIt uses GetEffectiveSpecularIntensity and GetEffectiveColor, they both return value multiplied by brightness:\r\n```\r\n/// Return effective specular intensity, multiplied by absolute value of brightness.\r\n    float GetEffectiveSpecularIntensity() const { return specularIntensity_ * Abs(brightness_); }\r\n```\r\n\r\nThen in shader, specular is multiplied by both cLightColor.rgb and cLightColor.a:\r\n\r\n`finalColor = diff * lightColor * (diffColor.rgb + spec * specColor * cLightColor.a);`\r\n\r\nI believe that this is wrong, and when brightness<1 specular is too dim, and when brightness>1 it is too bright.\r\n\r\nI've checked deferred and forward, haven't checked prepass(but it has hacky specular anyway :) ) and PBR, maybe PBR need EffectiveSpecularIntensity? Otherwise changing Batch.cpp to GetSpecularIntensity() might be a best solution. "},"comment":{"url":"https://api.github.com/repos/urho3d/Urho3D/issues/comments/363090495","html_url":"https://github.com/urho3d/Urho3D/issues/2252#issuecomment-363090495","issue_url":"https://api.github.com/repos/urho3d/Urho3D/issues/2252","id":363090495,"user":{"login":"JSandusky","id":5519639,"avatar_url":"https://avatars0.githubusercontent.com/u/5519639?v=4","gravatar_id":"","url":"https://api.github.com/users/JSandusky","html_url":"https://github.com/JSandusky","followers_url":"https://api.github.com/users/JSandusky/followers","following_url":"https://api.github.com/users/JSandusky/following{/other_user}","gists_url":"https://api.github.com/users/JSandusky/gists{/gist_id}","starred_url":"https://api.github.com/users/JSandusky/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/JSandusky/subscriptions","organizations_url":"https://api.github.com/users/JSandusky/orgs","repos_url":"https://api.github.com/users/JSandusky/repos","events_url":"https://api.github.com/users/JSandusky/events{/privacy}","received_events_url":"https://api.github.com/users/JSandusky/received_events","type":"User","site_admin":false},"created_at":"2018-02-05T13:52:48Z","updated_at":"2018-02-05T13:52:48Z","author_association":"CONTRIBUTOR","body":"> Then in shader, specular is multiplied by both cLightColor.rgb and cLightColor.a:\r\nfinalColor = diff * lightColor * (diffColor.rgb + spec * specColor * cLightColor.a);\r\nI believe that this is wrong, and when brightness<1 specular is too dim, and when brightness>1 it is too bright.\r\n\r\nThey're used separately within the shader so multiplying them separately makes sense. That code does look appropriate. \r\n\r\nIs there an unexpected interplay that adjusting *Specular Intensity* independently of brightness or the *material specular power* (it's the alpha channel of MatSpecColor) doesn't work well?\r\n\r\n> I've checked deferred and forward, haven't checked prepass(but it has hacky specular anyway :) ) and PBR, maybe PBR need EffectiveSpecularIntensity? Otherwise changing Batch.cpp to GetSpecularIntensity() might be a best solution.\r\n\r\nIt doesn't use it, everything is shiny."}}
{"actor":{"display_login":"JSandusky"}}