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

VideoCommon: normalize light direction #394

Merged
merged 1 commit into from Aug 4, 2014

Conversation

degasus
Copy link
Member

@degasus degasus commented May 23, 2014

It seems that the lighting direction must be normalized. This fixes lots of lighting issues mostly shown in the d3d backend.

@MayImilae
Copy link
Contributor

Screenshots from galop1n:

bug - http://i.imgur.com/xVYxgVl.png
correct - http://i.imgur.com/3jqdFLs.png

@neobrain
Copy link
Member

Can we have a hwtest for this?

@OussamaDanba
Copy link
Contributor

Apart from fixing the SMG2 issue this also fixes issue 6125.

@delroth
Copy link
Member

delroth commented May 23, 2014

And a fifolog please :)

On Fri, May 23, 2014 at 4:18 PM, Tony Wasserka notifications@github.comwrote:

Can we have a hwtest for this?


Reply to this email directly or view it on GitHubhttps://github.com//pull/394#issuecomment-44014336
.

Pierre "delroth" Bourdon delroth@gmail.com
Software Engineer @ Zürich, Switzerland
http://code.delroth.net/

@Linktothepast
Copy link
Contributor

Perhaps Issue 6269 is also relevant?

@OussamaDanba
Copy link
Contributor

This also seems to fix issue 6269. I can no longer reproduce it after applying the patch.

@JMC47
Copy link
Contributor

JMC47 commented May 23, 2014

It'll also fix Network Transmission, 6543

@MayImilae
Copy link
Contributor

Can someone (including myself) remember to close these issues when this PR is pushed?

@degasus
Copy link
Member Author

degasus commented May 26, 2014

@delroth Just pick any fifo log of this issue reports ;)

@neobrain Do you think we should move this normalization to the CPU? It's easy to implement at https://github.com/dolphin-emu/dolphin/blob/master/Source/Core/VideoCommon/VertexShaderManager.cpp#L272

@delroth
Copy link
Member

delroth commented May 26, 2014

Any point to move that to the CPU? Trading CPU load for GPU load here
sounds like a fine idea.

On Mon, May 26, 2014 at 1:22 PM, Markus Wick notifications@github.comwrote:

@delroth https://github.com/delroth Just pick any fifo log of this
issue reports ;)

@neobrain https://github.com/neobrain Do you think we should move this
normalization to the CPU? It's easy to implement at
https://github.com/dolphin-emu/dolphin/blob/master/Source/Core/VideoCommon/VertexShaderManager.cpp#L272


Reply to this email directly or view it on GitHubhttps://github.com//pull/394#issuecomment-44180396
.

Pierre "delroth" Bourdon delroth@gmail.com
Software Engineer @ Zürich, Switzerland
http://code.delroth.net/

@degasus
Copy link
Member Author

degasus commented May 26, 2014

@delroth This won't be a 1:1 trade. It can be done once per vertex (or pixel) on the gpu or once per lighting parameter changes on the cpu.

@delroth
Copy link
Member

delroth commented May 26, 2014

Good point, I should think before replying :)

On Mon, May 26, 2014 at 1:52 PM, Markus Wick notifications@github.comwrote:

@delroth https://github.com/delroth This won't be a 1:1 trade. It can
be done once per vertex (or pixel) on the gpu or once per lighting
parameter changes on the cpu.


Reply to this email directly or view it on GitHubhttps://github.com//pull/394#issuecomment-44182296
.

Pierre "delroth" Bourdon delroth@gmail.com
Software Engineer @ Zürich, Switzerland
http://code.delroth.net/

@galop1n
Copy link
Contributor

galop1n commented May 26, 2014

It would be possible to do the normalize on the cpu as it is bare constant, but it is not like we are suffering of gpu issues, and we cannot say the same of cpu.

@neobrain
Copy link
Member

@degasus Should be normalized on the CPU indeed. If not for performance reasons, then for the sake of keeping the shader code a bit cleaner.

@neobrain
Copy link
Member

By the way, is the software renderer already normalizing this properly?

@degasus
Copy link
Member Author

degasus commented May 26, 2014

@neobrain yeah, we need some hw test for this issue. All current fifologs should also play fine on videosw as it has the same overflow result.

@degasus degasus mentioned this pull request Jun 5, 2014
@degasus
Copy link
Member Author

degasus commented Jun 27, 2014

As #439 is merged, I've updated this PR to normalize on the CPU.

@JMC47
Copy link
Contributor

JMC47 commented Jun 27, 2014

LGTM; the dffs of the glitchy lighting show up fine now.

@neobrain
Copy link
Member

Please change the PR summary to be an actual summary rather than a novel squeezed into a single line.

@degasus degasus changed the title Render: Fix lighting bug with specular attenuation, the gx normalize the... VideoCommon: normalize light direction Jul 3, 2014
It seems that the lighting direction must be normalized. This fixes lots of lighting issues mostly shown in the d3d backend.
@degasus
Copy link
Member Author

degasus commented Jul 3, 2014

PR summary and commit msg changed.

@neobrain
Copy link
Member

neobrain commented Jul 4, 2014

LGTM, but I suggest against merging this before a hwtest is written. We have no real policy for this, but maybe we can start having one.

delroth added a commit that referenced this pull request Aug 4, 2014
VideoCommon: normalize light direction
@delroth delroth merged commit 15920d0 into dolphin-emu:master Aug 4, 2014
@Linktothepast Linktothepast mentioned this pull request Jan 10, 2015
7 tasks
@degasus degasus deleted the d3d_lighting_fix branch February 3, 2019 22:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
8 participants