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 ambient occlusion and dark lines at mapblock borders #6838

Closed
wants to merge 6 commits into from

Conversation

Projects
None yet
9 participants
@numberZero
Copy link
Contributor

numberZero commented Dec 25, 2017

Fixes #6578 and #6574

Mod to test: https://github.com/numberZero/minetest-test_lighting
Say /testlight and it will present all possible solid/transparent combinations (of size 2³), with one transparent node being full-sized nodebox and others being air.

UPD: found a case where a bug remains. Fixed.
screenshot_20171226_001225

UPD: that affects non-solid nodes per se as well (don’t say my torches are broken, I know that myself): Fixed.
screenshot_20171226_001717

@numberZero numberZero changed the title Fix ambient occlusion [WIP] Fix ambient occlusion Dec 25, 2017

@DTA7

This comment has been minimized.

Copy link
Contributor

DTA7 commented Dec 25, 2017

@numberZero Why do you do this in the same manner for solid nodes as for non-solid nodes (without face-dependence)?
Thanks for the test cases!

@numberZero

This comment has been minimized.

Copy link
Contributor Author

numberZero commented Dec 25, 2017

without face-dependence

@DTA7 There is face dependence. My code assumes light at a corner of a face of a solid node is the same as light at the corresponding corner of the (always non-solid) node in front of that particular face. That should always be the case.

By the way, I feel I fixed the bug.
screenshot_20171226_005441
The light spot is there because one node propagates light, and there is a torch inside.

@DTA7

This comment has been minimized.

Copy link
Contributor

DTA7 commented Dec 25, 2017

@numberZero Oooh, that's a good idea! I closed my pull request.
screenshot_20171225_232056
This however still remains (I think only for the solid node), do you see what I mean?
Somehow, the daylight lighting values are a little different, but they were not before this pull request (and mine), which seems to be only visible at night.

@numberZero

This comment has been minimized.

Copy link
Contributor Author

numberZero commented Dec 25, 2017

Both 02bc55c and 707dda6 do the same, so both should fix these bugs. They differ in style only.

@DTA7 Of course they are different. There is less sunlight below the box. Your old code have not considered that value at all IIRC, but that’s a straight way to various problems.

@numberZero

This comment has been minimized.

Copy link
Contributor Author

numberZero commented Dec 25, 2017

To clarify: my PR lets light wrap around nodes. While that may sound weird, that’s actually the way light behaves in MT.
screenshot_20171226_013321

@DTA7

This comment has been minimized.

Copy link
Contributor

DTA7 commented Dec 25, 2017

@numberZero OK, the higher value is indeed above, I was confused because the "boundary" in the interpolation was not always on the same height.

@numberZero numberZero changed the title [WIP] Fix ambient occlusion Fix ambient occlusion Dec 28, 2017

@numberZero

This comment has been minimized.

Copy link
Contributor Author

numberZero commented Dec 28, 2017

Er, this is not WIP anymore; forgot to change the title, sorry.

@sfan5 sfan5 removed the WIP label Dec 28, 2017

@paramat

This comment has been minimized.

Copy link
Member

paramat commented Dec 30, 2017

Will test.

@paramat

This comment has been minimized.

Copy link
Member

paramat commented Dec 31, 2017

Tested and it works.
Since i don't understand the code changes the 2nd approval should be from a dev who does.

@Fixer-007

This comment has been minimized.

Copy link
Contributor

Fixer-007 commented Dec 31, 2017

Seems good to me, only time will tell if there are side-effects 👍

@lhofhansl

This comment has been minimized.

Copy link
Contributor

lhofhansl commented Jan 2, 2018

I can't claim to understand what's going on here, but looks good generally.

Personally I'm very interested in having light propagate out from a source in a circle rather than what looks like a square today. (Looking at the code I understand why this happens and how something else would be more CPU intensive.)

@numberZero

This comment has been minimized.

Copy link
Contributor Author

numberZero commented Jan 2, 2018

@paramat paramat removed the One approval label Jan 2, 2018

@paramat

This comment has been minimized.

Copy link
Member

paramat commented Jan 4, 2018

Tested new version.
This fixes the incorrect nodebox-node ambient occlusion of #6578
This PR also now fixes the dark lines at mapblock borders of #6574 (comment)
The new approach to these bugfixes is described here #6765 (comment)
👍

screenshot_20180104_012915

else if (add_node(k + 4).light_propagates)
obstructed[3] = false;
}
std::array<bool, 4> obstructed = {{ 1, 1, 1, 1 }};

This comment has been minimized.

Copy link
@nerzhul

nerzhul Jan 4, 2018

Member

is this not better to use a bitset here ?

This comment has been minimized.

Copy link
@numberZero

numberZero Jan 4, 2018

Author Contributor

bitset is size-optimized, not speed-optimized

This comment has been minimized.

Copy link
@paramat

paramat Jan 4, 2018

Member

Seems optimum then, ok for me.

obstructed[1] = opaque1 && opaque3;
obstructed[2] = opaque2 && opaque3;
for (int k = 0; k < 3; ++k)
if (add_node(k + 4, obstructed[k]))

This comment has been minimized.

Copy link
@nerzhul

nerzhul Jan 4, 2018

Member

k + 4 ? can we have a constant ?

This comment has been minimized.

Copy link
@paramat

paramat Jan 4, 2018

Member

But k + 4 isn't consant?

This comment has been minimized.

Copy link
@numberZero

numberZero Jan 4, 2018

Author Contributor

this number is a part of the algorithm, not some random constant.

This comment has been minimized.

Copy link
@paramat

paramat Jan 4, 2018

Member

Yes this is fine as it is.

obstructed[3] = false;
if (add_node(7, obstructed[3])) { // wrap light around nodes
ambient_occlusion -= 3;
for (int k = 0; k < 3; ++k)

This comment has been minimized.

Copy link
@nerzhul

This comment has been minimized.

Copy link
@numberZero

numberZero Jan 4, 2018

Author Contributor

done

obstructed[0] = opaque1 && opaque2;
obstructed[1] = opaque1 && opaque3;
obstructed[2] = opaque2 && opaque3;
for (int k = 0; k < 3; ++k)

This comment has been minimized.

Copy link
@nerzhul

This comment has been minimized.

Copy link
@numberZero

numberZero Jan 4, 2018

Author Contributor

why?

This comment has been minimized.

Copy link
@paramat

paramat Jan 4, 2018

Member

We tend to prefer the smallest integer type instead of an 'int'.

This comment has been minimized.

Copy link
@numberZero

numberZero Jan 4, 2018

Author Contributor

That’s a local variable, so it will probably use the whole 8-byte (4-byte on a 32-bit system) register anyway. But would it be u8, the compiler would have to make sure it uses lower 8 bits only, because unsigned overflows must work that way (that shouldn‘t cause any runtime overhead here, but only because the bounds are compile-time constants). OTOH, signed overflows are undefined behavior, so the compiler never needs to care of that.

This comment has been minimized.

This comment has been minimized.

Copy link
@paramat

paramat Jan 4, 2018

Member

(I am no expert on the subject)

This comment has been minimized.

Copy link
@numberZero

numberZero Jan 4, 2018

Author Contributor

okay, done

@numberZero

This comment has been minimized.

Copy link
Contributor Author

numberZero commented Jan 4, 2018

@nerzhul fixed.

@SmallJoker

This comment has been minimized.

Copy link
Member

SmallJoker commented Jan 7, 2018

Looks good so far but it's not entirely fixed. Here a cobblestone node before placing (top) and after removing it again (bottom). This only happens on a mapblock border.
grafik

@numberZero

This comment has been minimized.

Copy link
Contributor Author

numberZero commented Jan 7, 2018

@SmallJoker I suppose the surface on your screenshot is on the mapblock boundary. If so, see #6765 for details.

@SmallJoker

This comment has been minimized.

Copy link
Member

SmallJoker commented Jan 7, 2018

@numberZero Never mind. I thought this PR would also fix that light issue, using a different approach..

@paramat

This comment has been minimized.

Copy link
Member

paramat commented Jan 7, 2018

@SmallJoker
The new approach to these bugfixes is described here #6765 (comment)
"I updated #6838, it should fix dark lines now, but some glitches will remain; this one now optionally fixes the glitches at the cost of more CPU load."
So this PR now fixes the extreme lines, and leaves very subtle glitches that are optionally fixed by #6765 at the expense of more neighbour blocks being updated. Read the #6765 thread for explanation.

@paramat paramat changed the title Fix ambient occlusion Fix ambient occlusion and dark lines at mapblock borders Jan 7, 2018

All done.

@SmallJoker

This comment has been minimized.

Copy link
Member

SmallJoker commented Jan 11, 2018

👍

@paramat

This comment has been minimized.

Copy link
Member

paramat commented Jan 12, 2018

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.