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

Cleanup in flat lighting #7051

Merged
merged 4 commits into from Mar 3, 2018

Conversation

Projects
None yet
5 participants
@numberZero
Copy link
Contributor

commented Feb 17, 2018

Fixes flat lighting part of #7040
screenshot_20180218_004553

////////////////////
EDIT by paramat:
More explanation for reviewers from #7040 (comment)

nodeboxes use light one level brighter for drawing than their true light value. But to do that, undiminish_light is used, which clamps at LIGHT_MAX, which is 14. So solid nodes get 15, and nodeboxes are drawn at 14 under direct sunlight.

So the difference in light levels only became visible now that light levels 14 and 15 are visibly different due to #6696

@numberZero

This comment has been minimized.

Copy link
Contributor Author

commented Feb 17, 2018

++increment;
}

if (light)

This comment has been minimized.

Copy link
@red-001

red-001 Feb 17, 2018

Contributor

imo it would be cleaner to write this as light != 0 as we are using it as a number not a boolean.

This comment has been minimized.

Copy link
@numberZero

numberZero Feb 18, 2018

Author Contributor

Or light > 0; it’s unsigned so that’s all the same.

@Fixer-007

This comment has been minimized.

Copy link
Contributor

commented Feb 18, 2018

Does not fix for me under direct light when smooth lighting is enabled it seems:
default

@numberZero

This comment has been minimized.

Copy link
Contributor Author

commented Feb 18, 2018

Also, one note on smooth lighting: it’s just the way it works. It can’t know that a face is exposed to direct sunlight, such logic would be complicated. I suppose that was imagined on adding the “non-trivial” label to the original issue.

@numberZero

This comment has been minimized.

Copy link
Contributor Author

commented Feb 18, 2018

So yes, it doesn’t fix that. Only flat lighting.

@paramat

This comment has been minimized.

Copy link
Member

commented Feb 19, 2018

The bug with smooth lighting under direct sunlight is very ugly so unfortunately i think we should revert 2d9f0d3 #6696 instead. Thanks for this but -1

@numberZero

This comment has been minimized.

Copy link
Contributor Author

commented Feb 19, 2018

@paramat This PR is a cleanup, and only then a bugfix, so should probably be merged in any case, if appropriate.

@numberZero numberZero changed the title Fix yet another lighting bug Cleanup in flat lighting Feb 20, 2018

@paramat

This comment has been minimized.

Copy link
Member

commented Feb 20, 2018

Ok so you mean merge this and revert #6696 ?
Please could you explain in the first comment what is being done here and why? What is removed and changed seems important.
Are 'diminish_light' and 'undimish_light' used elsewhere other than for nodeboxes?
'getInterorLight' may be used for several things already, are they all checked?
If #6696 is reverted then why is this PR needed?

@paramat

This comment has been minimized.

Copy link
Member

commented Feb 20, 2018

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

12:21 numzero paramat: (on #7051) diminish_light and undiminish_light were used in several places a long time ago; but currently, they are only used in getInteriorLight. And the way they are used is such that they can be replaced with just a few lines of code.
12:21 ShadowBot #7051 -- Cleanup in flat lighting by numberZero
12:22 numzero Direct replacement would be clamping light to LIGHT_MAX, then, if it is non-zero, adding increment and then clamping to LIGHT_MAX again.
12:22 numzero That would work exactly the same as the current code
12:24 numzero My PR clamps to LIGHT_SUN instead, to fix #7040 for flat lighting. (The first clamping disappears as the original light level is never greater than LIGHT_SUN)
12:24 ShadowBot #7040 -- Dark and miscoloured nodeboxes
12:28 numzero In the case #6696 is reverted, LIGHT_SUN and LIGHT_MAX are eqivalent again. So the only difference will be that light=LIGHT_SUN, increment=-1 will be treated as LIGHT_MAX (full brightness) instead of LIGHT_MAX-1
12:28 ShadowBot #6696 -- Update light decoding table size by numberZero
12:28 numzero by so far, increment=1 is only ever used for selection box...
12:29 numzero Nevertheless, I have yet another idea how to do things in a better way; working on that right now.

@Fixer-007

This comment has been minimized.

Copy link
Contributor

commented Feb 22, 2018

I've applied it, but not sure what to check without smooth lighting, do you mean this weird thing (patch applied)? vvv
default
default
default

@numberZero

This comment has been minimized.

Copy link
Contributor Author

commented Feb 22, 2018

@Fixer-007 This should fix dark slabs (snow, mainly) that you mentioned in #7040 and not introduce any new bugs.

@Fixer-007

This comment has been minimized.

Copy link
Contributor

commented Feb 22, 2018

Snow slabs without smooth lighting? Yes, they looked more or less normal under sunlight and at night + torch without smooth lighting (and with smooth lighting too, after applying 7061).

@paramat

This comment has been minimized.

Copy link
Member

commented Feb 23, 2018

@Fixer-007 see #7040 (comment) onwards and numberZero's reply, that weirdness you see has always been there and is unavoidable with nodeboxes as explained:

Maybe this is because nodeboxes get a single light level from inside and uses that for all faces, whereas cubic nodes have each face getting it's light level from the node neightbour?

I'm beginning to wonder if it's always been like this and we didn't notice due to: 1. no smooth lighting on nodeboxes so they looked different anyway. 2. Appearence in direct sunlight was ok due to light levels 14 and 15 being the same. 3. When nodeboxes were given smooth lighting the smooth lighting disguised the difference.

@paramat

This comment has been minimized.

Copy link
Member

commented Feb 23, 2018

Code looks good, will now test.

@paramat

This comment has been minimized.

Copy link
Member

commented Feb 23, 2018

👍 Fixes brightness mismatch in direct sunlight with smooth lighting disabled.

@SmallJoker SmallJoker merged commit f5fd4a0 into minetest:master Mar 3, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

minduser00 added a commit to minduser00/minetest that referenced this pull request Mar 18, 2018

minduser00 added a commit to minduser00/minetest that referenced this pull request Mar 18, 2018

osjc added 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
You can’t perform that action at this time.