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

Skip face culling in shadows for double-sided materials (e.g. plantlike) #13500

Merged
merged 2 commits into from
Sep 17, 2023

Conversation

x2048
Copy link
Contributor

@x2048 x2048 commented May 13, 2023

At some sun angles, plantlike nodes produce incomplete or no shadow. This is because they render double-sided (no face culling) but shadow renderer does not respect that and culls front faces.

This PR fixes this problem by only overriding culling for nodes where at least one side is culled in the original material.

To do

This PR is Ready for Review.

How to test

  1. Use API to set negative tilt for the sun/moon
minetest.register_on_joinplayer(function(player)
	player:set_sky({body_orbit_tilt = -30})
end)
  1. Enter the world and look at the shadows of plantlike nodes. Shadows must be correct at all times.
  2. Check shadows of other nodes, they should also be correct.

@x2048 x2048 added @ Client / Audiovisuals Trivial The change is a trivial bug fix, documentation or maintenance change, as per the Git Guidelines Bugfix 🐛 PRs that fix a bug labels May 13, 2023
@x2048 x2048 added this to the 5.8.0 milestone May 13, 2023
@x2048 x2048 self-assigned this May 13, 2023
@lhofhansl
Copy link
Contributor

Seems to work. :)

@lhofhansl
Copy link
Contributor

Actually... This seems to also remove shadows from water.

@lhofhansl lhofhansl added the Action / change needed Code still needs changes (PR) / more information requested (Issues) label May 16, 2023
@Zughy
Copy link
Member

Zughy commented Jun 18, 2023

@x2048 any updates on this?

Copy link
Member

@SmallJoker SmallJoker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did not notice any difference with plantlike and rooted plantlike.

@SmallJoker SmallJoker added One approval ✅ ◻️ and removed Action / change needed Code still needs changes (PR) / more information requested (Issues) labels Jul 7, 2023
@grorp
Copy link
Member

grorp commented Jul 8, 2023

Two screenshots of the problem described by lhofhansl:

Before this PR:
screenshot before this PR

After this PR:
screenshot after this PR

F5 debug info:
screenshot of F5 debug info

Screenshots taken in Minetest Game, but with the dynamic shadow intensity variation stuff removed. Instead:

player:set_lighting({ shadows = { intensity = 0.85 } })

minetest.conf excerpt:

antialiasing = ssaa
shadow_map_color = true
shadow_map_texture_size = 8192
shadow_map_max_distance = 300
enable_dynamic_shadows = true
shadow_levels = 5
shadow_map_texture_32bit = true
enable_waving_plants = false
shadow_filters = 2
enable_waving_water = false
enable_waving_plants = false
enable_waving_leaves = false
tone_mapping = false
enable_shaders = true
mip_map = true
time_speed = 0
/time 18:00

@sfan5 sfan5 added the Action / change needed Code still needs changes (PR) / more information requested (Issues) label Jul 8, 2023
@SmallJoker
Copy link
Member

SmallJoker commented Jul 8, 2023

Cannot reproduce the water shadow issue. I am using the following dynamic shadows settings:

shadow_filters = 1
shadow_soft_radius = 5
shadow_map_texture_32bit = true
shadow_map_texture_size = 1024
enable_dynamic_shadows = true
shadow_map_color = false
shadow_map_max_distance = 100

diff

EDIT: also using tone_mapping = true but turning it off does not make a difference either.

@grorp
Copy link
Member

grorp commented Jul 8, 2023

Here's a simpler example:

Before:
screenshot before this PR
The player has two shadows: One on the water surface and a smaller one on the bottom of the pond.

After:
screenshot after this PR
The player has only one shadow, the one on the water surface. The player still has two shadows, you just don't see both because of the dark texture. The bottom of the pond is completely covered by a shadow, as if the water also had a shadow.

EDIT: Actually, maybe I'm wrong... The new behavior (that the water has a shadow as well) seems like it should be the expected one with colored shadows.

/time 12:30

@lhofhansl
Copy link
Contributor

With PR:
screenshot_20230719_161837

Without PR:
screenshot_20230719_161734

Looks different to me.
-1

@x2048 x2048 removed the Action / change needed Code still needs changes (PR) / more information requested (Issues) label Aug 3, 2023
@sfan5
Copy link
Member

sfan5 commented Aug 6, 2023

@grorp or @lhofhansl if you have a moment please re-test

@lhofhansl
Copy link
Contributor

The problem I was seeing is gone now. Shadows on water are correct now.

@grorp
Copy link
Member

grorp commented Aug 21, 2023

The problem I was seeing is fixed as well (it was the same problem).

@lhofhansl
Copy link
Contributor

@x2048 Wanna merge?

Copy link
Member

@srifqi srifqi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested this PR and it fixes the issue. Plant-like nodes cast correct shadows. Shadows on water (liquid) surface and inside water look brighter than master branch.

Test master branch This PR
Plant-like shadows plant-like shadow on master plant-like shadow on this PR
Shadows on water (liquid) surface and inside water shadows on/in water on master shadows on/in water on this PR

Contents of minetest.conf:

enable_shaders = true
antialiasing = ssaa

shadow_filters = 1
shadow_levels = 5
shadow_map_color = false
shadow_map_max_distance = 140
shadow_map_texture_32bit = true
shadow_map_texture_size = 2048

enable_dynamic_shadows = true

@grorp
Copy link
Member

grorp commented Aug 31, 2023

Shadows on water (liquid) surface and inside water look brighter than master branch.

On your second screenshot, all shadows look brighter, whether in the water or outside. Could the brightness difference possibly be caused by MTG's shadow intensity randomization (minetest/minetest_game#3024)?

Just tried again, this PR doesn't change anything related to water shadows for me:

shadow_map_color = false shadow_map_color = true
current master branch screenshot, master branch screenshot, master branch, colored shadows
this PR, rebased to current master branch screenshot, this PR screenshot, this PR, colored shadows

All the screenshots look nearly identical to me.

@grorp
Copy link
Member

grorp commented Sep 11, 2023

@srifqi Would you be fine with merging this?

@srifqi
Copy link
Member

srifqi commented Sep 12, 2023

Would you be fine with merging this?

Yes.


Could the brightness difference possibly be caused by MTG's shadow intensity randomization (minetest/minetest_game#3024)?

I guess. The problem disappears in other games.

@lhofhansl
Copy link
Contributor

@x2048 Still around?

@x2048 x2048 merged commit e36b222 into minetest:master Sep 17, 2023
13 checks passed
@x2048 x2048 deleted the plant-shadows branch September 17, 2023 19:42
@x2048
Copy link
Contributor Author

x2048 commented Sep 17, 2023

@lhofhansl still around, not active.

@x2048 x2048 restored the plant-shadows branch October 1, 2023 20:17
kawogi pushed a commit to kawogi/minetest that referenced this pull request Dec 19, 2023
…ke) (minetest#13500)

* Skip face culling in shadows for double-sided materials (e.g. plantlike)

* Keep previous face culling for transparent surfaces e.g. water
cosin15 pushed a commit to cosin15/minetest that referenced this pull request Feb 24, 2024
…ke) (minetest#13500)

* Skip face culling in shadows for double-sided materials (e.g. plantlike)

* Keep previous face culling for transparent surfaces e.g. water
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bugfix 🐛 PRs that fix a bug @ Client / Audiovisuals Testing needed Trivial The change is a trivial bug fix, documentation or maintenance change, as per the Git Guidelines >= Two approvals ✅ ✅
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants