Add modulate property to lightmaps.

[Skywolf285]

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

• Closes Vulkan: LightmapGI energy not implemented (unlike BakedLightmapData in 3.x) #49968
• Offers a potential solution for Add an option to bake only ambient occlusion (AO) with LightmapGI godot-proposals#11596

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

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]

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.

[Skywolf285]

@Calinou

Implementing it that way has a few issues:

1. Opening the scene (be it from loading it in game or loading in editor) makes the lightmap appear entirely black as the modulate variable doesn't get set in LightStorage::Lightmap. If there is a way to set this on scene load this is probably fixable.
2. If the LightmapData is used in the same scene more than once will changes made to one LightmapGI also affect the other while the modulate property on the other stays unchanged. Meaning they get out of sync. When the modulate property is part of the LightmapData they don't desync and I also feel it's more obvious that this is expected behavior. That said, reusing LightmapData is a very niche use case so maybe not worth worrying about?

Again, would love to have some feedback on this.

[Calinou]

1. Opening the scene (be it from loading it in game or loading in editor) makes the lightmap appear entirely black as the modulate variable doesn't get set in LightStorage::Lightmap. If there is a way to set this on scene load this is probably fixable.

I find this strange, as this behavior seems to work fine with the specular property added to #109737.

2. If the LightmapData is used in the same scene more than once will changes made to one LightmapGI also affect the other while the modulate property on the other stays unchanged. Meaning they get out of sync. When the modulate property is part of the LightmapData they don't desync and I also feel it's more obvious that this is expected behavior. That said, reusing LightmapData is a very niche use case so maybe not worth worrying about?

Indeed, that's a limitation of storing the property in both places. That said, I don't expect LightmapGIData resources to be shared across multiple LightmapGI nodes (this never happens if you only rely on the Bake Lightmaps button).

[Skywolf285]

I find this strange, as this behavior seems to work fine with the specular property added to #109737.

Yeah, my bad. Missed calling update_modulate under LightmapGI::set_light_data(). It works now and have pushed the change.

Indeed, that's a limitation of storing the property in both places. That said, I don't expect LightmapGIData resources to be shared across multiple LightmapGI nodes (this never happens if you only rely on the Bake Lightmaps button).

For my specific use case I might actually end up doing this but since you can still access the modulate value directly using RenderingServer to get the "true" value is this easy to work around. That said, should it be clarified somewhere within the documentation that the module property "desyncing" when re-using LightmapGIData resources is expected behavior?

[Calinou]

That said, should it be clarified somewhere within the documentation that the module property "desyncing" when re-using LightmapGIData resources is expected behavior?

Yes, I would mention it in the LightmapGIData property's description.

[Calinou]

Tested locally, it works as expected in all renderers. Code looks good to me.

Testing project: test_pr_120641.zip

lightmapgi_modulate_day_night_cycle.webm

[Calinou]

Suggested change

The color the lightmap is multiplied with. Gets applied in real-time without the need to re-bake the lightmap.

The color the lightmap is multiplied with. Gets applied in real-time without the need to re-bake the lightmap. This can be used to tint indirect lighting (for lights baked with the [constant Light3D.BAKE_DYNAMIC] bake mode). This can be useful for day/night cycles, with the lightmap baked while the [DirectionalLight3D] node is at its "noon" position. Keep in mind other baked light sources (such as environment lighting or the light emitted by emissive materials) will also be affected by the tint.

[Skywolf285]

...with the lightmap baked while the [DirectionalLight3D] node is at its "noon" position.

Am kinda confused by this one.

Won't this also bake the shadows of this DirectionalLight3D? I guess the mention of Light3D.BAKE_DYNAMIC implies this light should be set up this way but then why would the positions of a light source not contributing to the lightmap matter at all? For sun direction of the WorldEnvironment?

Or am I misunderstanding how Light3D.BAKE_DYNAMIC interacts with lightmaps? The description for BAKE_DYNAMIC states it only works for VoxelGI and SDFGI.

In all my tests I just set a custom environment for the LightMapGI that's a solid white color. This worth mentioning as an alternative for setting up day/night cycles?

[allenwp]

I'm not sure if this is a desirable enough feature to be worth the extra shader uniform, code, and maintenance, so I can't comment on whether this is a good idea to merge. But I have some notes on Color usage:

Although Color may be used to store values of any encoding, the red (r), green (g), and blue (b) properties of Color are expected by Godot to be encoded using the nonlinear sRGB transfer function unless otherwise stated. This color encoding is used by many traditional art and web tools, making it easy to match colors between Godot and these tools.

Source: https://docs.godotengine.org/en/stable/classes/class_color.html

I haven't tested this, but from looking at the code, it appears that new public lightmap_set_modulate function (and its getter) use linear-encoded colour. Would it be possible to adjust all public-facing APIs to use Color that is encoded with the nonlinaer sRGB transfer function instead? This simplifies the interface for the user by making Color consistent throughout Godot. Ideally, this conversion from sRGB to linear happens as close as possible to when it is passed into the shader so we don't need to add notes about Color being linear-encoded to every function that passes it around internally.

I also see you have a note on the modulate property saying // in linear space. -- This is good, but it's better to say // uses linear encoding instead of "space", because the term "[colour] space" is fairly ambiguous, but Godot uses the term "encoding" to describe the difference between linear and nonlinear sRGB encoding. Any parts of the code where Color is stored or passed in linear encoding needs to be marked like you did here.

[Skywolf285]

The reason I did things this way was to avoid doing the conversion every frame in RenderForwardClustered::_setup_lightmaps as that seemed wasteful to me. If that's not a problem I could do the conversion in there.

Else we can just keep the LightStorage::Lightmap::modulate property in linear space encoding and do the conversion in the set/get functions instead. Otherwise do I fully agree this should be in line with all other public facing functions taking/returning Color values.

[allenwp]

Else we can just keep the LightStorage::Lightmap::modulate property in linear space encoding and do the conversion in the set/get functions instead.

This is what I had in mind. I think that means we have minimal use of linear-encoded Color in both the public and internal APIs and we aren’t doing the conversion every frame, correct?

(Performance takes priority, but if it’s the same performance with less linear-encoded Color, then that’s the ideal because it means less documentation and less chance for confusion over the required encoding.)

[Ivorforce]

Hello, thank you for opening this pull request, and your effort on it so far.

Unfortunately, there are several things not quite aligned right now:

• Our policy prohibits new features being implemented by new contributors. (this policy was recently added; I apologize if this comes as a surprise)
• You created the PR immediately after opening the proposal. We need proposals to garner a healthy amount of attention and discussion before they can be considered for approval.
• You mentioned in your proposal that you are planning a project, not actually creating it. This contradicts our guidelines for new features.

Therefore, we are closing the pull request.

If you would like to contribute to Godot, please consider starting with smaller and simpler contributions.

Thank you again for your work and sharing it with the Godot community. If/when all of the above are resolved, we can reconsider this feature.

[Skywolf285]

1. This PR was opened before these policy changes were published. So I had no way of knowing. If I had I wouldn't have bothered.

2. My proposal was created over a month ago and since it got positive reaction in the from of github's reaction function and since other people have asked for similar functionality in the past I figured that was enough.

3. I'm not going to start a project that needs a feature that is not in the engine. With planning I meant looking into if my vision for my game is even possible with the current toolset. This PR also fixes a regression where the energy property was removed after 3.x. A regression that, if it wasn't there, I wouldn't even need to make this PR.

I hope you reconsider reopening this PR as I feel the reasoning for closing it is deeply unfair and doesn't reflect the needs of the users (not just me).

[Zireael07]

PR opened 25 June, policy changes announced 30 June, so I think closing this is unfair indeed

[akien-mga]

The point about the new policy is one out of 3 points Ivorforce mentioned. The remaining points still stand and mean this PR currently doesn't pass the threshold for "should reviewers spend time on this, and should we take on the burden of maintaining it forever if this is merged":

• There is no proven demand for the feature. The proposal for this change has very little engagement, and another older and slightly more requested feature is linked as potentially helped by this change, but it's not clear how.
• There is no project in development making use of this feature to confirm that it actually works well. OP mentions that they are planning a project which would require this feature. That's not a sufficient reason to add a feature in Godot. But OP is welcome to use this implementation in a custom build of Godot to develop their project, and if it's proven to work well and the demand has been confirmed in the meantime, we're happy to reconsider the PR.

Closing this PR doesn't mean we reject the feature forever, but currently the proposal doesn't meet our criteria for reviewers' limited and already bottlenecked time.

[Skywolf285]

I would agree with you here if 3.x didn't already have a similar feature. Feels weird to me if restoring functionality lost during the transition to a new engine version is deemed as potential bloat unless the removal had a very exlicit reason. (Which to my knowledge wasn't the case here, else #49968 should be closed as an intended change).

In godotengine/godot-proposals#11596 Calinou mentioned modulation would offer a potential solution. Hence why I listed it here.

I am fully aware the chances of me changing the outcome of this are very slim here. I am just trying to understand the logic as to what seperates my PR from something like #89919 which was in a similar situation.