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

Disabled Lights Are Included in Lightmap #82834

Open
sandsalamand opened this issue Oct 5, 2023 · 17 comments
Open

Disabled Lights Are Included in Lightmap #82834

sandsalamand opened this issue Oct 5, 2023 · 17 comments

Comments

@sandsalamand
Copy link

sandsalamand commented Oct 5, 2023

Godot version

4.1.2 stable mono

System information

Windows 10.0.19045 - Vulkan (Forward+) - dedicated NVIDIA GeForce RTX 2070 SUPER (NVIDIA; 30.0.15.1123) - Intel(R) Core(TM) i7-9700K CPU @ 3.60GHz (8 Threads)

Issue description

Invisible lights (tested with OmniLight3D and SpotLight3D) are included in the baked lightmap.

Expected: disabled/invisible lights are not included in the lightmap.

Steps to reproduce

  1. Place an OmniLight3D or SpotLight3D in the scene.
  2. Nest it below a LightMapGI
  3. Disable the Light
  4. Place a MeshInstance3D in the scene (in my case, saved as a scene, but not sure if that matters).
  5. Bake. Lightmap settings don't seem to change anything, but my last test used Low + 3 Bounces + Directional ON + Interior OFF + Use Denoiser ON.

Minimal reproduction project

N/A

@jsjtxietian
Copy link
Contributor

Hi, by invisible lights do you mean toggle visibility at scene hierachy or change the bake mode to disabled? If the first situation, the doc said "Keep in mind hiding a light will have no effect for baking, so this must be used instead of hiding the Light node." https://docs.godotengine.org/en/latest/tutorials/3d/global_illumination/using_lightmap_gi.html

However, I do think invisible light in the scene hierarchy should not contribute to lightmap baking.

@Calinou
Copy link
Member

Calinou commented Oct 5, 2023

This may be done by design, so that you can maximize performance by having lights that only affect baked surfaces with no real-time computation. That said, #73289 is probably a better solution as it'll also affect direct light in LightmapProbes.

@Calinou Calinou added discussion and removed bug labels Oct 5, 2023
@jsjtxietian
Copy link
Contributor

jsjtxietian commented Oct 5, 2023

Well I would slightly argue that it's not intuitive that invisible light is also contributing to lightmap baking. Other than this it's fine.

At least support hidding light when baking is trivial. If @sandsalamand you really need this, I think just add a condition here for a quick fix:

if (light && light->get_bake_mode() != Light3D::BAKE_DISABLED) {

@sandsalamand
Copy link
Author

sandsalamand commented Oct 5, 2023

@Calinou So from reading that issue, are you saying that Static Baked lights would be completely invisible in the editor until baked? And thus, they would solve the use case of wanting lights that are invisible in the editor but included in the lightmap?

If so, I agree. If that option was available, then we could safely change it so that invisible dynamic lights are not included in the lightmap.

@sandsalamand
Copy link
Author

@jsjtxietian It's not essential right now for my project, but it's definitely unintuitive behavior that should (IMO) be changed.

I read that documentation page, but I didn't notice that part. Maybe it's because of the use of the word "hidden" instead of "invisible". Also, if we're keeping the current behavior, it should probably be in a warning box to call more attention to it.

@Calinou
Copy link
Member

Calinou commented Oct 5, 2023

@Calinou So from reading that issue, are you saying that Static Baked lights would be completely invisible in the editor until baked? And thus, they would solve the use case of wanting lights that are invisible in the editor but included in the lightmap?

This would be an additional bake mode for the reasons I described in #73289 (comment). The existing Static bake mode would not be affected by this change, as it still has some use cases (such as more precise direct lighting for direct objects, and the ability for them to receive real-time shadows from baked objects).

@sandsalamand
Copy link
Author

Yes, I understand. So, should we add the fix which @jsjtxietian mentioned to your fork?

@Calinou
Copy link
Member

Calinou commented Oct 6, 2023

Yes, I understand. So, should we add the fix which @jsjtxietian mentioned to your fork?

I would prefer we make the changes separately, as it has important implications. My branch is still far from being finished (and I don't know if I'll be able to complete it) anyway.

@atirut-w
Copy link
Contributor

This is very similar to a problem I encountered where editor-only lights are included in lightmaps. Could it be the same issue as this?

@Calinou
Copy link
Member

Calinou commented Oct 23, 2023

This is very similar to a problem I encountered where editor-only lights are included in lightmaps. Could it be the same issue as this?

This can likely be the fixed the same way in the lightmapper's light gathering code. However, unlike hidden lights, I consider including editor-only lights in the bake a bug for sure.

@rptfrg
Copy link
Contributor

rptfrg commented Mar 21, 2024

I have attached a reproduction project, I hope it could be useful.

Lightmap Bug.zip

image

@sandsalamand

This comment was marked as off-topic.

@Calinou

This comment was marked as off-topic.

@sandsalamand

This comment was marked as off-topic.

@rptfrg

This comment was marked as off-topic.

@Zireael07

This comment was marked as off-topic.

@sandsalamand

This comment was marked as off-topic.

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

No branches or pull requests

7 participants