1330 add active component for all kinds of disabling purposes

[RodrigoVintem]

Description

add active component for all kinds of disabling purposes

Checklist

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

[github-actions]

PR Preview Action v1.4.8
🚀 Deployed preview to https://GameDevTecnico.github.io/cubos/preview/pr-1357/
on branch gh-pages at 2024-11-12 15:09 UTC

[github-actions]

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

[github-actions]

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

[github-actions]

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

[github-actions]

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

[github-actions]

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

[github-actions]

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

[github-actions]

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

[github-actions]

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

[github-actions]

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

[codecov]

Codecov Report

❌ Patch coverage is 0% with 148 lines in your changes missing coverage. Please review.
✅ Project coverage is 41.89%. Comparing base (8d1d78c) to head (6a10308).
⚠️ Report is 303 commits behind head on main.

Files with missing lines	Patch %	Lines
.../render/cascaded_shadow_maps_rasterizer/plugin.cpp	0.00%	73 Missing ⚠️
engine/src/render/lights/plugin.cpp	0.00%	22 Missing ⚠️
engine/src/render/deferred_shading/plugin.cpp	0.00%	11 Missing ⚠️
engine/src/render/camera/plugin.cpp	0.00%	8 Missing ⚠️
engine/src/render/cascaded_shadow_maps/plugin.cpp	0.00%	8 Missing ⚠️
engine/src/tools/debug_camera/plugin.cpp	0.00%	6 Missing ⚠️
engine/src/render/shadow_atlas/plugin.cpp	0.00%	4 Missing ⚠️
engine/src/render/split_screen/plugin.cpp	0.00%	4 Missing ⚠️
engine/src/render/active/plugin.cpp	0.00%	3 Missing ⚠️
...gine/src/render/shadow_atlas_rasterizer/plugin.cpp	0.00%	3 Missing ⚠️
... and 3 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1357      +/-   ##
==========================================
- Coverage   41.95%   41.89%   -0.07%     
==========================================
  Files         441      443       +2     
  Lines       33115    33163      +48     
  Branches     3870     3881      +11     
==========================================
  Hits        13893    13893              
- Misses      19222    19270      +48     

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

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[tomas7770]

What's already implemented LGTM! Here's what should need to be done for the lights. (GitHub doesn't let me add comments to unchanged lines of code so I'll try to do this manually)

engine/src/render/deferred_shading/plugin.cpp

In each of the loops that initialize lights in the shader (there is one for each type of light: directional, point, spot), check if the light is active, and continue if it's not.

for (auto [lightLocalToWorld, light, lightActive, caster] : directionalLights)
{
    if (!lightActive.active)
    {
        continue;
    }
    auto& perLight = perScene.directionalLights[perScene.numDirectionalLights++];

engine/src/render/shadow_atlas/plugin.cpp

No need to reserve space in the shadow atlas for lights that aren't active.

for (auto [caster, active] : casters)
{
    if (!active.active)
    {
        continue;
    }
    bool foundSlot = false;

The entity that has the caster is the same one that has the light, so adding the Active component to the caster query allows checking if the light is active.

.call([](ShadowAtlas& atlas, Query<SpotShadowCaster&, const Active&> casters) {

engine/src/render/cascaded_shadow_maps/plugin.cpp

Same as above.

for (auto [caster, active] : query)
{
    if (!active.active)
    {
        continue;
    }
    // Remove shadow maps for cameras that no longer exist

engine/src/render/shadow_atlas_rasterizer/plugin.cpp

No need to draw shadows for lights that aren't active.

for (auto [caster, light, localToWorld, active] : lights)
{
    if (!active.active)
    {
        continue;
    }
    // Get light viewport

engine/src/render/cascaded_shadow_maps_rasterizer/plugin.cpp

Same as above.

for (auto [caster, light, localToWorld, lightActive] : lights)
{
    if (!lightActive.active)
    {
        continue;
    }
    auto& shadowMap = caster.shadowMaps.at(cameraEntity);

---

Little sidenote unrelated to the PR.

engine/src/render/ssao/plugin.cpp has several includes that don't seem to be used for anything, such as lights and HDR. Maybe these can be removed?

[RodrigoVintem]

Added Tomas' changes to the files related to light, that should use the Active component. Also removed the unnecessary includes mentioned by Tomas.

[SrGesus]

Otherwise LGTM!

[mkuritsu]

Overall looks good to me just very small changes to maintain consistency.

Aside from that I'm just concern about some implications of this implementation.

[tomas7770]

There are still some small things that should be changed, but otherwise looks good!

[RodrigoVintem]

All the new changes proposed by the reviewers have been made.

[RiscadoA]

Mostly LGTM, pointed out some details that we should sort out before merging.
Ty for working on this 😌

[RiscadoA]

I don't think this plugin should be a render-plugin. It is also relevant outside it. We'll use it for audio, and probably for collisions too later on. So, it would be better to put it in engine-plugin, and move to active dir to cubos/engine/

[RiscadoA]

I think you also need to add cubos.depends(cameraPlugin) here, and the same goes for the other plugins.
Have you tested running the shadows sample? It probably crashes

[RiscadoA]

Since this has been abandoned I will pick it up on the next milestone.