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

Add point light shadows #1366

Conversation

tomas7770
Copy link
Contributor

Description

Implements shadows for point lights.

Checklist

  • Self-review changes.
  • Evaluate impact on the documentation.
  • Ensure test coverage.
  • Write new samples.
  • Add entry to the changelog's unreleased section.

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

⚠️ Clang-Tidy found issue(s) with the introduced code (1/1)

engine/src/render/shadow_atlas_rasterizer/plugin.cpp Outdated Show resolved Hide resolved
engine/src/render/deferred_shading/plugin.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

github-actions bot commented Nov 16, 2024

PR Preview Action v1.4.8
🚀 Deployed preview to https://GameDevTecnico.github.io/cubos/preview/pr-1366/
on branch gh-pages at 2024-11-29 14:18 UTC

Copy link

codecov bot commented Nov 17, 2024

Codecov Report

Attention: Patch coverage is 0% with 290 lines in your changes missing coverage. Please review.

Project coverage is 53.47%. Comparing base (6508bfc) to head (cf69124).
Report is 9 commits behind head on main.

Files with missing lines Patch % Lines
...ine/src/render/shadows/atlas_rasterizer/plugin.cpp 0.00% 146 Missing ⚠️
engine/src/render/shadows/atlas/plugin.cpp 0.00% 66 Missing ⚠️
engine/src/render/deferred_shading/plugin.cpp 0.00% 28 Missing ⚠️
engine/src/render/shadows/atlas/point_atlas.cpp 0.00% 17 Missing ⚠️
engine/src/render/shadows/atlas/spot_atlas.cpp 0.00% 16 Missing ⚠️
core/src/geom/utils.cpp 0.00% 5 Missing ⚠️
engine/src/render/shadows/casters/plugin.cpp 0.00% 5 Missing ⚠️
...include/cubos/engine/render/shadows/atlas/slot.hpp 0.00% 4 Missing ⚠️
engine/src/render/defaults/plugin.cpp 0.00% 1 Missing ⚠️
engine/src/render/shadows/cascaded/plugin.cpp 0.00% 1 Missing ⚠️
... and 1 more
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1366      +/-   ##
==========================================
- Coverage   53.70%   53.47%   -0.24%     
==========================================
  Files         450      451       +1     
  Lines       25691    25802     +111     
  Branches     2375     2386      +11     
==========================================
  Hits        13797    13797              
- Misses      11894    12005     +111     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@tomas7770 tomas7770 force-pushed the 1188-add-support-for-point-shadow-casters-in-shadow-map-atlas-plugin branch from 5008557 to a83b9c8 Compare November 17, 2024 11:33
@tomas7770 tomas7770 marked this pull request as ready for review November 17, 2024 11:51
@tomas7770 tomas7770 requested review from RiscadoA and a team as code owners November 17, 2024 11:51
@tomas7770 tomas7770 requested review from Docas95, Scarface1809 and RodrigoVintem and removed request for a team November 17, 2024 11:51
Copy link
Member

@RiscadoA RiscadoA left a comment

Choose a reason for hiding this comment

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

Very good work on this 🚀 👀 that was quick
Other than the comments I added, I feel like it would be a good idea to split the shadow atlas resource and plugin into two if you implement it like this.
The code is already quite complex as it is, and with this you're essentially merging two different rendering phases into a single one. It would be simpler to read and understand if they were separate, I think.
Maybe a spot_shadow_atlas and a point_shadow_atlas? Or should we keep it as is?

engine/src/render/deferred_shading/plugin.cpp Outdated Show resolved Hide resolved
engine/src/render/shadow_atlas_rasterizer/plugin.cpp Outdated Show resolved Hide resolved
engine/src/render/shadow_atlas_rasterizer/plugin.cpp Outdated Show resolved Hide resolved
@RiscadoA RiscadoA removed the request for review from RodrigoVintem November 17, 2024 17:27
@RiscadoA RiscadoA added this to the 0.5 milestone Nov 23, 2024
@tomas7770 tomas7770 force-pushed the 1188-add-support-for-point-shadow-casters-in-shadow-map-atlas-plugin branch from e6f64e1 to 615addf Compare November 25, 2024 17:14
@tomas7770 tomas7770 requested review from fallenatlas and a team as code owners November 25, 2024 17:14
@tomas7770 tomas7770 requested review from joaomanita and removed request for a team November 25, 2024 17:14
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

⚠️ Clang-Tidy found issue(s) with the introduced code (1/1)

engine/src/render/deferred_shading/plugin.cpp Show resolved Hide resolved
core/src/geom/utils.cpp Outdated Show resolved Hide resolved
core/src/geom/utils.cpp Outdated Show resolved Hide resolved
core/src/geom/utils.cpp Outdated Show resolved Hide resolved
core/src/geom/utils.cpp Outdated Show resolved Hide resolved
@tomas7770 tomas7770 force-pushed the 1188-add-support-for-point-shadow-casters-in-shadow-map-atlas-plugin branch 2 times, most recently from f9b9304 to f5f8920 Compare November 26, 2024 19:45
@tomas7770 tomas7770 removed the request for review from fallenatlas November 26, 2024 19:47
@tomas7770 tomas7770 removed the request for review from joaomanita November 26, 2024 19:47
Copy link
Member

@RiscadoA RiscadoA left a comment

Choose a reason for hiding this comment

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

LGTM!

core/src/geom/utils.cpp Outdated Show resolved Hide resolved
@tomas7770 tomas7770 force-pushed the 1188-add-support-for-point-shadow-casters-in-shadow-map-atlas-plugin branch from f5f8920 to 2eccc83 Compare November 29, 2024 14:15
@tomas7770 tomas7770 force-pushed the 1188-add-support-for-point-shadow-casters-in-shadow-map-atlas-plugin branch 2 times, most recently from b488733 to 939c533 Compare November 29, 2024 14:21
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

⚠️ Clang-Tidy found issue(s) with the introduced code (1/1)

core/src/geom/utils.cpp Show resolved Hide resolved
@tomas7770 tomas7770 force-pushed the 1188-add-support-for-point-shadow-casters-in-shadow-map-atlas-plugin branch from 939c533 to cf69124 Compare November 29, 2024 14:55
@tomas7770 tomas7770 merged commit 99302b3 into main Nov 29, 2024
11 checks passed
@tomas7770 tomas7770 deleted the 1188-add-support-for-point-shadow-casters-in-shadow-map-atlas-plugin branch November 29, 2024 15:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for point shadow casters in shadow map atlas plugin
4 participants