Skip to content

Add modulate property to lightmaps.#120641

Open
Skywolf285 wants to merge 1 commit into
godotengine:masterfrom
Skywolf285:master
Open

Add modulate property to lightmaps.#120641
Skywolf285 wants to merge 1 commit into
godotengine:masterfrom
Skywolf285:master

Conversation

@Skywolf285

Copy link
Copy Markdown

What problem(s) does this PR solve?

Adds a color property to lightmaps to multiple the final lightmap by. Effectively giving a limited way to alter the final lighting in real-time. Mostly useful to create day-night cycles but can have various other uses as well.

I already wrote a more detailed reasoning in godotengine/godot-proposals#14722

Eyecandy, using just LightmapGI and a few gradients to control the sky and lightmap color:
output

Additional information

Will fully admit, I never touched C++ code so this entire thing is written using pattern observation, my best educated guess work and looking up basic C++ information whenever the syntax didn't make sense.

For the most part I'm pretty certain I did things the right way. The only thing I'm not certain about is how to update the lightmap captures. The problem I'm running into is that, to my knowledge, at no point a lightmap is aware what lightmap instances use it. Which is a problem because LightStorage::Lightmap::modulate has to be part of LightStorage::Lightmap to be accessible in the shader while RendererSceneCull needs a lightmap instance to get the RendererSceneCull::InstanceLightmapData to call RendererSceneCull::_instance_queue_update on each of its geometries so the lightmap captures get properly updated.

So in order for lightmap captures to update whenever LightStorage::lightmap_set_modulate is called, did I store the lightmap instances for each lightmap in LightStorage::Lightmap::lightmap_instances. This feels like a very bad, hacky and error prone way to do things and requires each time a lightmap is assigned or removed to/from a lightmap instance LightStorage::lightmap_insert_to_lightmap_instances and/or LightStorage::lightmap_erase_from_lightmap_instances to be called.

I considered other alternatives but each has their own drawbacks. Would love to have feedback on this. There is very non-zero chance I missed a better option here!

Also not sure about the UX. Because LightStorage::Lightmap::modulate is part of the lightmap does it appear under Data/light_data as part of the LightmapGIData rather than under Tweaks where I feel it would be more logical. But to store the modulate property in both the LightmapGI node and the LightmapGIData and carefully having to cover each scenario where these two might get out of sync doesn't sound great either. Not to mention it making it confusing where the modulate property is actually used.

@Calinou

Calinou commented Jun 25, 2026

Copy link
Copy Markdown
Member

Also not sure about the UX. Because LightStorage::Lightmap::modulate is part of the lightmap does it appear under Data/light_data as part of the LightmapGIData rather than under Tweaks where I feel it would be more logical. But to store the modulate property in both the LightmapGI node and the LightmapGIData and carefully having to cover each scenario where these two might get out of sync doesn't sound great either. Not to mention it making it confusing where the modulate property is actually used.

Check how #109737 stores the specular intensity property in LightmapGI instead of LightmapGIData.

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.

Vulkan: LightmapGI energy not implemented (unlike BakedLightmapData in 3.x)

3 participants