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

Enhance tile skirts light occlusion #11460

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

calogeromauceri
Copy link
Contributor

I'm proposing the following solution for this issue #11459
The tile skirst are causing artifacts on shadows when the light is tanget to the tile, i.e. when it is near the horizon.

The tangent light should be occluded, but it leaks from below the terrain tile. The skirt is casting a thin shadow that is causing artifacts on the final result.

The solution I'm proposing is to increase the skirt size an to add a plane to the bottom of the tile, in that way the light cannot pass through the side/bottom of it and the terrain is better shadowed.

Here is a sandcastle example with the default behavior (advance the time till the artifacts are visible)
sandcastle

sandcastle

and this is the result with the proposed solution

sandcastle_desired

Here is an example of the tile with the longer skirts

Screenshot 2023-08-07 17 47 21
Screenshot 2023-08-07 17 47 27

Let me know if the proposed fix makes sense or if there is any better solution

@cesium-concierge
Copy link

Thanks for the pull request @calogeromauceri!

  • ✔️ Signed CLA found.
  • CHANGES.md was not updated.
    • If this change updates the public API in any way, please add a bullet point to CHANGES.md.
  • ❔ Unit tests were not updated.
    • Make sure you've updated tests to reflect your changes, added tests for any new code, and ran the code coverage tool.

Reviewers, don't forget to make sure that:

  • Cesium Viewer works.
  • Works in 2D/CV.

@ggetz
Copy link
Contributor

ggetz commented Aug 11, 2023

Thanks for the suggestion @calogeromauceri!

@lilleyse Would you mind taking a look at the overall approach here?

@lilleyse
Copy link
Contributor

I wonder if this can also be solved by disabling back face culling for terrain shadow map commands, i.e. set this line to false.

@calogeromauceri
Copy link
Contributor Author

calogeromauceri commented Aug 17, 2023

thanks @lilleyse for looking into this. I tried the solution you suggested, but it seems to me disabling it does not work well in some circumstances.
For example in the sandcastle example I posted above, I see light leaking where there should not be

Screenshot 2023-08-17 14 45 44

and here is another view where the skirts artifacts are still visible

Screenshot 2023-08-17 14 56 10

ezgif com-video-to-gif

sandcastle

Using the longer skirts approach I suggested, seems to produce better shadows

ezgif com-video-to-gif (2)

Maybe an hybrid approach using long skirts and disabling back face culling for terrain shadow map commands would produce even better results? I'm not an expert just testing some trivial changes, you might have a better insight knowledge

@cesium-concierge
Copy link

Thanks again for your contribution @calogeromauceri!

No one has commented on this pull request in 90 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 90 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

@malaretv
Copy link
Contributor

@lilleyse @ggetz are there any blockers to merging this in? Or any alternate approaches worth trying? Would love to collaborate on them if so

@ggetz
Copy link
Contributor

ggetz commented Mar 22, 2024

@lilleyse Do you have any other suggestions on the approach here? Or is this the most straightforward given the backface culling idea doesn't seem to be working in all cases?

@lilleyse
Copy link
Contributor

Is it possible to enable long skirts and bottom plane only if terrain shadow casting is enabled? That way this change doesn't affect performance in the general case.

@calogeromauceri
Copy link
Contributor Author

@lilleyse @ggetz thanks for looking into this. I'll update the code as you suggested

@calogeromauceri
Copy link
Contributor Author

@lilleyse I've had the chance to delve into the implementation of the changes you suggested regarding enabling long skirts and a bottom plane exclusively when terrain shadow casting is enabled. However, since the tile mesh, including skirt vertices, is generated at load time, dynamically adjusting them based on shadow casting isn't feasible.

As an alternative, we could add a flag to the terrain provider upon creation that enables these features. Do you think this solution fits our needs or do you think there are better alternatives?

@lilleyse
Copy link
Contributor

lilleyse commented Apr 3, 2024

As an alternative, we could add a flag to the terrain provider upon creation that enables these features. Do you think this solution fits our needs or do you think there are better alternatives?

That would be ok, I think.

@calogeromauceri
Copy link
Contributor Author

calogeromauceri commented Apr 5, 2024

@lilleyse @ggetz I added a flag called extendedSkirts to CesiumTerrainProvider for enabling extended skirts. The flag is false by default. If it is set to true then the extended skirts are used: longer skirts and bottom tile plane for better light occlusion when the terrain shadows are enabled.

Here is a simple sandcastle example on how to use it

@calogeromauceri
Copy link
Contributor Author

@lilleyse @ggetz I finally found some time for checking and fix the tests failing.
I would appreciate your feedback on these changes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants