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

Fix node-nodebox lighting difference in direct sunlight #7061

Merged
merged 5 commits into from
Mar 17, 2018
Merged

Fix node-nodebox lighting difference in direct sunlight #7061

merged 5 commits into from
Mar 17, 2018

Conversation

numberZero
Copy link
Contributor

@numberZero numberZero commented Feb 20, 2018

Fixes smooth lighting part of #7040

  1. In getSmoothLightCombined (which is called for each corner of the node), it now checks for direct sunlight (light level of 15). If it is detected, full brightness (255) is used instead of the average. Then, ambient occlusion is applied, if necessary; otherwise it returns 255 for the day light bank.
  2. MapblockMeshGenerator::getSmoothLightFrame checks for that value and marks the corresponding corner and its bottom counterpart (for convenience) as exposed to direct sunlight.
  3. MapblockMeshGenerator::blendLight now calculates two values for the day light bank: a base light value (used for sides), computed as usual, and a boosted value (used for sun-facing quads), computed considering marked corners as having full light.
  4. LightInfo::getPair blends these values using given factor, for which dot product of sun direction (0, 1, 0) and face normal, clamped to [0, 1], is used. This way, sides and bottom faces are not affected, but top faces get full sunlight. This feature is only used for nodeboxes and mesh nodes.

Note: lighting settings on screenshot are extreme to make defects, if any, more noticeable. That’s the reason for such dark shadow.
screenshot_20180220_160414

@paramat paramat added the WIP label Feb 20, 2018
@numberZero numberZero changed the title [WIP] Fix regression after #6696 Fix regression after #6696 Feb 20, 2018
@Fixer-007
Copy link
Contributor

Do I need to apply 7051 as well?

@numberZero
Copy link
Contributor Author

numberZero commented Feb 22, 2018 via email

@Fixer-007
Copy link
Contributor

Fixer-007 commented Feb 22, 2018

Checked this PR in game, it fixes my problems with smooth lighting (on snow and slab+full) 👍

@paramat paramat removed the WIP label Feb 23, 2018
@paramat
Copy link
Contributor

paramat commented Feb 23, 2018

From IRC http://irc.minetest.net/minetest-dev/2018-02-20#i_5234396

16:05 numzero okay, now #7061 works for nodeboxes and meshnodes; other cuboid-using nodes (framed glass) are affected but should be fine as well
16:05 ShadowBot #7061 -- Fix regression after #6696 by numberZero
16:07 numzero I saw a glitch on extreme settings (lighting_beta=4.0 and plain white texture) but even in such conditions it is barely noticeable
16:07 numzero it also may be GPU-specific
16:08 numzero as it is fixed by moving one multiplication from the vertex shader to the fragment one (with unlimited precision, that move would not change anything)
16:10 numzero but without shaders such change is not possible ofc
16:12 numzero note that it doesn’t affect flat lighting, my previous PR fixes that

@paramat
Copy link
Contributor

paramat commented Feb 23, 2018

Don't understand how the code works but will test.

@paramat
Copy link
Contributor

paramat commented Feb 25, 2018

Please can you help reviewers by explaining in the first post what this does, how it works and why it works this way? We're short on reviewer time and available reviewers may not be good with this area of code, if i understand how this works i may be able to +1 it.

@numberZero
Copy link
Contributor Author

numberZero commented Feb 25, 2018

@paramat Done. I should probably document that in the source briefly, though. UPD: done that as well

@paramat
Copy link
Contributor

paramat commented Mar 2, 2018

Sorry for delay, i will test this as soon as possible.

@rubenwardy
Copy link
Member

Change set looks good, although I'm not familiar with this area of the code

@SmallJoker
Copy link
Member

Side-by-side comparison using the default settings:
screenshot_20180310_095322

Looks much better now.

@paramat
Copy link
Contributor

paramat commented Mar 14, 2018

Tested your branch, there is a bug with smooth lighting disabled but i assume that's because #7051 is not included in this branch.

With smooth lighting and a white snow texture, snow slabs next to snowblocks, identical whiteness:

screenshot_20180314_013941

screenshot_20180314_014004

@paramat
Copy link
Contributor

paramat commented Mar 14, 2018

screenshot_20180314_020252

^ Facing W

screenshot_20180314_020403

^ These lines align to NW-SE (player facing NW)

Ambient occlusion is different for different corners.

@paramat
Copy link
Contributor

paramat commented Mar 14, 2018

Maybe i should test master plus your patch instead.

@paramat paramat changed the title Fix regression after #6696 Fix node-nodebox lighting difference in direct sunlight Mar 14, 2018
@paramat
Copy link
Contributor

paramat commented Mar 14, 2018

Applying patch to master, same appearence, all else looks fine though:

screenshot_20180314_022814

@paramat
Copy link
Contributor

paramat commented Mar 14, 2018

In master appearence is the same, so this is not caused by your mod:

screenshot_20180314_023600

So this PR tests fine.

@numberZero
Copy link
Contributor Author

Ambient occlusion is different for different corners.

@paramat It isn’t. Each quad is drawn as a pair of triangles (check the wireframe view), and the same diagonal is used for that split, regardless of node position; that is, it is aligned identically relative to the world, but therefore differently relative to the corners. That’s not a (new) bug if these are slabs; there was some code to choose the diagonal based on lighting to make gradients look nicer, but it was for fast-faces (solid nodes) only. It might be possible to do the same for nodeboxes, however.

@paramat
Copy link
Contributor

paramat commented Mar 15, 2018

Yes i suspected that was nothing new, good to know thanks.
Your PR tests fine then, will look at code again.

@paramat
Copy link
Contributor

paramat commented Mar 16, 2018

I'll look at code then will probably +1 and merge it.

@paramat
Copy link
Contributor

paramat commented Mar 16, 2018

Thanks to your description i have a rough understanding of this.
👍

@nerzhul nerzhul merged commit 0358ae7 into minetest:master Mar 17, 2018
@nerzhul
Copy link
Member

nerzhul commented Mar 17, 2018

Merged, thanks

minduser00 pushed a commit to minduser00/minetest that referenced this pull request Mar 18, 2018
minduser00 pushed a commit to minduser00/minetest that referenced this pull request Mar 18, 2018
@numberZero numberZero deleted the boost-sunlight branch April 11, 2018 22:39
osjc pushed a commit to osjc/minetest that referenced this pull request Jan 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants