godot icon indicating copy to clipboard operation
godot copied to clipboard

Expose runtime baking functionality in LightmapGI

Open sourcelocation opened this issue 1 year ago • 19 comments

(This PR is a duplicate of https://github.com/godotengine/godot/pull/91676 with the requested changes implemented. This was done since the creator of that PR is no longer as interested in this functionality as he was before, hence I'm making my own PR)

This will resolve https://github.com/godotengine/godot/issues/59217 This partially resolves https://github.com/godotengine/godot-proposals/issues/8656 . It exposes bake() for scripting, however there isn't any cancelling or aborting yet. Works for exported projects.

Implementation notes:

  • As .exr files cannot be imported at runtime, I save as a .res. This is also because when the lightmapper is available in exported projects, we can use .res files, while we can't save .exr files
  • The radiance changes setting to Radiance 128, and then resetting back to what it was, is because the lightmaps were not generating correctly at runtime using the Scene/Custom Sky environment options. Setting it to 128 fixes this. I am not sure what is causing this, possibly something related to environment_bake_panorama or sky_bake_panorama. There is a related Size2i parameter for this radiance size, but setting that to 256 while using a radiance size of 256 doesn't fix this issue.
  • module_lightmapper_rd_enabled build parameter has to be enabled for runtime baking to work
  • module_xatlas_unwrap_enabled, build parameter has to be enabled for runtime unwrapping to work

Demo video:

https://github.com/user-attachments/assets/542a1b5e-07b9-4b0d-a7df-668516359543

Project for testing: https://github.com/sourcelocation/lightmapruntimebakingtest/

This is my first ever contribution to Godot, by the way 🙂

sourcelocation avatar Jul 30 '24 22:07 sourcelocation

Hey, I think you forgot to clean up some of the original PR text. For clarity, this does work in exported projects, correct?

sinewavey avatar Jul 30 '24 23:07 sinewavey

Yes it does. I kept the original PR text because I found it important, but have also added my own notes down below (the lines talking about build parameters explain lightmap baking at runtime)

sourcelocation avatar Jul 31 '24 07:07 sourcelocation

Nevermind, I see what you were talking about. I edited the PR text, my bad

sourcelocation avatar Jul 31 '24 07:07 sourcelocation

You need to include the original author as a co-author, like so (the empty line is necessary):


Co-authored-by: Rising Thumb <[email protected]>

Copy this exactly to the end of your commit message with git commit --amend

I will add some review comments that I had as pending on the original later

Also just confirming that the original author has actually abandoned it, it doesn't look like it as they were active on it just 3 days ago and doesn't seem to be abandoned

AThousandShips avatar Jul 31 '24 07:07 AThousandShips

You didn't copy what I suggested properly, it needs to be after an empty like at the end of the message, you made it one single line

AThousandShips avatar Jul 31 '24 08:07 AThousandShips

@AThousandShips This should work. Thank you for the help on this one!

So the original author of that PR hasn't "abandoned" the PR entirely, but was stuck in a weird state, where it had multiple commits not amended, author having only email as a form of contacting and told me this being not his # 1 priority in private. So with all of this combined I thought creating a separate PR would be appropriate?

I'll make sure to double check with him however. There's no rush since this isn't going to be merged until 4.3 comes out anyway 🙂

sourcelocation avatar Jul 31 '24 08:07 sourcelocation

I already asked on the original PR so let them respond there for completeness

AThousandShips avatar Jul 31 '24 08:07 AThousandShips

Screenshot_20240731-133959.png

Got the response from the original PR creator. 😃

sourcelocation avatar Jul 31 '24 10:07 sourcelocation

So I made a typo forgetting to call SWAP macro when TOOLS_ENABLED is false, causing exported projects to fail baking lighting. I hope this was the last change before merge lol

sourcelocation avatar Jul 31 '24 18:07 sourcelocation

Fixed the following issues:

I get an error printed every time lightmaps are baked in a project:

ERROR: Condition "!EditorSettings::get_singleton() || !EditorSettings::get_singleton()->has_setting(p_setting)" is true. Returning: Variant() at: _EDITOR_GET (./editor/editor_settings.cpp:1289)

Fixed. And as a result of fixing this, the release build no longer crashes. OIDN denoiser is still not implemented. I left a TODO comment, hopefully someone who's more experienced can implement it in the future 🙂

sourcelocation avatar Aug 16 '24 10:08 sourcelocation

I'm sorry if I sound pushy here, as I don't know how long PR merges on Godot Engine repo take, and I don't know if I should've said it's ready for review again, but I'd appreciate if any of maintainers could review this PR 🙂

sourcelocation avatar Aug 19 '24 17:08 sourcelocation

this looks awesome! keep up the good work.

ExplodingImplosion avatar Aug 21 '24 19:08 ExplodingImplosion

LGTM

boredcoder411 avatar Aug 21 '24 19:08 boredcoder411

I desperately need this too! This is gonna be a game changer for automation

StefanH-AT avatar Sep 10 '24 23:09 StefanH-AT

Hi again. When can we expect to see this PR reach the master branch?

sourcelocation avatar Sep 19 '24 05:09 sourcelocation

Please don't bump issues and PRs unnecessarily. The PR still needs to be reviewed and approved by the rendering team, which has a big backlog, so please have patience.

Users looking forward to this change can show their supporting by adding a :+1: to the OP, and ideally, test the pull request and confirm that it does what you need in your projects.

akien-mga avatar Sep 19 '24 11:09 akien-mga

Hi,

I tried to test this earlier, and in editor, it it largely was nice.

Unfortunately, my real use case for this is to 'foolproof' a map import process for users creating maps and then baking lightmaps (for something similar, see things like q3map2).

With the same script used to test in editor, I don't seem to be able to bake from a headless editor instance; the images returned are always null.

I'd guess this is likely unsupported use, but I wanted to double check if there were any additional concerns, unless I'm making any other mistakes here.

Example script:

func _ready() -> void:
	if OS.get_cmdline_user_args().has("--bake"):
		bake.call_deferred()
		
func bake() -> void:
	var filename = "bake"+Time.get_time_string_from_system()+".lmbake"
	filename = filename.validate_filename()
	print("Baking lightmap (to %s)..." % (filename))
	var error := lightmap_gi.bake(self, filename)
	prints("Bake completed:", error)
	return

sinewavey avatar Sep 22 '24 22:09 sinewavey

@sinewavey Hi, thanks for testing. I tested runtime baking only in editor, as well as exported builds, as headless baking wasn't relevant for my project. I'll try looking into this issue on the weekends out of curiosity. However, just in case I don't manage to find time, would it be possible to merge this PR anyway (and include a note in docs saying headless builds are unsupported)?

sourcelocation avatar Sep 23 '24 20:09 sourcelocation

However, just in case I don't manage to find time, would it be possible to merge this PR anyway (and include a note in docs saying headless builds are unsupported)?

I think this limitation is fine as long as this is documented. Adding headless baking support is welcome, but it sounds nontrivial considering it means local RenderingDevices would need to be functional even when the headless DisplayServer is used. Normally, no rendering context is created in this situation.

We did manage to have Vulkan contexts created when the editor is using OpenGL, so it's still likely technically feasible.

As a workaround, you could minimize the editor/project window as soon as it's started, and spawn it with a tiny size with borderless enabled (and disable quit requests so that you can't Alt + F4 it).

Calinou avatar Sep 23 '24 21:09 Calinou

Stopping development of this PR due to recent actions taken by the Godot team. You can merge or close, but I won't be finishing it.

sourcelocation avatar Sep 29 '24 21:09 sourcelocation

Ok, if further work is needed on this PR, I'll be happy to open a new PR to address any further changes that are needed to take this over the finish line. Feel free to mention me if anything is needed

RisingThumb avatar Sep 30 '24 00:09 RisingThumb

@RisingThumb it seems you may need to open a new PR so runtime baking stuff here can be considered as this issue is closed.

NHodgesVFX avatar Dec 02 '24 18:12 NHodgesVFX