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

Force-update shadows when the world is changed #12364

Merged
merged 3 commits into from
May 26, 2022

Conversation

x2048
Copy link
Contributor

@x2048 x2048 commented May 21, 2022

This PR attempts to fix the "lagging shadows" problem when shadow_update_frames setting is greater than 1.

shadow_update_frames setting was introduced to mitigate the performance degradation caused by shadow mapping and spreads rendering of the 'map' shadow over the given number of frames. This delays change in the shadow when the map changes (mining or building) and causes artifacts such as nodes not casting shadows on building or shadows cast by air on mining.

This PR forces a full SM render on the frame where a map change occurs, removing the lag completely.

The downside is that shadow update frequency is not even, which may be noticeable with fast-moving shadows (e.g. shadows of high cliffs).

To do

This PR is a Ready for Review.

How to test

  1. Set shadow_update_frames to 16
  2. Start a world and start digging/building/pour water etc.
  3. Notice that shadows are updated immediately

@x2048 x2048 added @ Client / Audiovisuals Discussion Issues meant for discussion of one or more proposals labels May 21, 2022
@lhofhansl
Copy link
Contributor

Is that even a problem worth fixing?

@rubenwardy rubenwardy added the Roadmap The change matches an item on the current roadmap. label May 23, 2022
@x2048
Copy link
Contributor Author

x2048 commented May 23, 2022

@lhofhansl
The problem is quite visible with shadow_update_frames > 8, and I have received some feedback supporting this.

There is performance gain in using shadow_update_frame, and this patch removes the main annoyance it brings.

@lhofhansl
Copy link
Contributor

I see. Makes sense.
I think we should also (in the other PR) then leave the shadow_update_frame at what it is now (8) and also allow 16.

@lhofhansl
Copy link
Contributor

Works.

src/client/client.cpp Outdated Show resolved Hide resolved
Copy link
Member

@sfan5 sfan5 left a comment

Choose a reason for hiding this comment

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

LGTM

@x2048 x2048 merged commit ef22c02 into minetest:master May 26, 2022
@x2048 x2048 deleted the shadows_frequency branch May 26, 2022 20:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@ Client / Audiovisuals Discussion Issues meant for discussion of one or more proposals Roadmap The change matches an item on the current roadmap. >= Two approvals ✅ ✅
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants