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

Expose runtime baking functionality in LightmapGI #94965

Conversation

sourcelocation
Copy link

@sourcelocation sourcelocation commented Jul 30, 2024

(This PR is a duplicate of #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 #59217
This partially resolves godotengine/godot-proposals#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:

352137190-3a768b3d-a05e-4001-9207-d0ef824f93a7.mp4

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

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

@sourcelocation sourcelocation requested review from a team as code owners July 30, 2024 22:14
@sinewavey

This comment was marked as resolved.

@sourcelocation

This comment has been minimized.

@sourcelocation
Copy link
Author

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

@AThousandShips
Copy link
Member

AThousandShips commented Jul 31, 2024

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

This comment was marked as resolved.

@sourcelocation sourcelocation force-pushed the expose-lightmap-baking branch from 6cebd00 to 5628210 Compare July 31, 2024 08:08
@sourcelocation

This comment has been minimized.

@AThousandShips
Copy link
Member

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

@sourcelocation sourcelocation force-pushed the expose-lightmap-baking branch 2 times, most recently from cd84c41 to d9501e2 Compare July 31, 2024 10:26
@sourcelocation

This comment has been minimized.

@sourcelocation
Copy link
Author

sourcelocation commented Jul 31, 2024

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

Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested locally, it mostly works as expected. The JNLM denoiser works as well.

Testing project: lightmapruntimebakingtest-bake-always.zip
This is the testing project from OP, but modified to bake every frame for use with Movie Maker mode. With this, you can almost1 make Godot a pathtraced video renderer:

movie_maker_lightmap.mp4

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)

My guess is that it's trying to access editor settings where some preferences are defined, but you can't access them in a running project. There should be equivalent project settings created for these properties instead (these would only be used at runtime, not in the editor).

For the reasons described above, OIDN can't be used as the editor setting that defines the path to OIDN can't be read. Runtime baking falls back to JNLM in this case. While using OIDN would be challenging in an exported project (since you'd need to package it with your game), I think it should be feasible when running a project from the editor. This means a project setting to define the OIDN path should likely be added.


When trying to bake from a release export template compiled with module_lightmapper_rd_enabled=yes module_xatlas_unwrap_enabled=yes, I get a crash. Backtrace from gdb (as Godot's crash handler isn't working here for some reason):

#0  RenderingDevice::_uniform_set_update_shared (this=this@entry=0x504d980, p_uniform_set=p_uniform_set@entry=0x0)
    at servers/rendering/rendering_device.cpp:2786
#1  0x000000000252e225 in RenderingDevice::draw_list_draw (this=0x504d980, p_list=<optimized out>, p_use_indices=true, p_instances=1, 
    p_procedural_vertices=0) at servers/rendering/rendering_device.cpp:4114
#2  0x00000000033a03f5 in RendererSceneRenderImplementation::RenderForwardClustered::_render_list_template<(RendererSceneRenderImplementation::RenderForwardClustered::PassMode)6, 0u> (this=0x4440000000b, p_draw_list=<optimized out>, p_framebuffer_Format=<optimized out>, 
    p_params=0x7fffffffaf90, p_from_element=0, p_to_element=<optimized out>)
    at servers/rendering/renderer_rd/forward_clustered/render_forward_clustered.cpp:518
#3  RendererSceneRenderImplementation::RenderForwardClustered::_render_list(long, long, RendererSceneRenderImplementation::RenderForwardClustered::RenderListParameters*, unsigned int, unsigned int) [clone .constprop.0] (this=this@entry=0x6ccfba0, 
    p_draw_list=p_draw_list@entry=576460752303423488, p_framebuffer_Format=<optimized out>, p_params=p_params@entry=0x7fffffffaf90, 
    p_to_element=p_to_element@entry=1, p_from_element=0)
    at servers/rendering/renderer_rd/forward_clustered/render_forward_clustered.cpp:573
#4  0x000000000270c819 in RendererSceneRenderImplementation::RenderForwardClustered::_render_uv2 (this=<optimized out>, p_instances=..., 
    p_framebuffer=..., p_region=...) at servers/rendering/renderer_rd/forward_clustered/render_forward_clustered.cpp:2822
#5  0x0000000002689841 in RendererSceneRenderRD::bake_render_uv2 (this=0x6ccfba0, p_base=..., p_material_overrides=..., p_image_size=...)
    at ./core/math/vector2i.h:148
#6  0x00000000025f3a6a in RendererSceneCull::bake_render_uv2 (this=<optimized out>, p_base=..., p_material_overrides=..., p_image_size=...)
    at servers/rendering/renderer_scene_cull.cpp:4205
#7  RenderingServerDefault::bake_render_uv2 (this=this@entry=0x4ede8e0, p1=..., p2=..., p3=...)
    at servers/rendering/rendering_server_default.h:838
#8  0x00000000033deab0 in LightmapGI::_bake(Node*, String, bool (*)(float, String const&, void*, bool), void*) [clone .constprop.0] (
    this=this@entry=0x96c2ee0, p_from_node=p_from_node@entry=0x96382b0, p_image_data_path=..., p_bake_userdata=0x0, 
    p_bake_step=<optimized out>) at scene/3d/lightmap_gi.cpp:806
#9  0x000000000182338a in LightmapGI::bake (this=0x96c2ee0, p_from_node=0x96382b0, p_image_data_path=...) at scene/3d/lightmap_gi.cpp:740
#10 0x000000000181e0e1 in call_with_variant_args_ret_helper<__UnexistingClass, LightmapGI::BakeError, Node*, String, 0ul, 1ul> (
    p_instance=<optimized out>, p_method=<optimized out>, p_args=<synthetic pointer>, r_ret=..., r_error=...)
    at ./core/variant/binder_common.h:758
#11 call_with_variant_args_ret_dv<__UnexistingClass, LightmapGI::BakeError, Node*, String> (p_instance=<optimized out>, 
    p_method=<optimized out>, p_args=<optimized out>, p_argcount=<optimized out>, r_ret=..., r_error=..., default_values=...)
    at ./core/variant/binder_common.h:535
#12 MethodBindTR<LightmapGI::BakeError, Node*, String>::call (this=<optimized out>, p_object=<optimized out>, p_args=<optimized out>, 
    p_arg_count=<optimized out>, r_error=...) at ./core/object/method_bind.h:524
#13 0x00000000030abdb1 in Object::callp (this=0x96c2ee0, p_method=..., p_args=0x7fffffffbe28, p_argcount=2, r_error=...)
    at core/object/object.cpp:808
#14 0x0000000002e8c811 in Variant::callp (this=<optimized out>, p_method=..., p_args=0x7fffffffbe28, p_argcount=2, r_ret=..., r_error=...)
    at core/variant/variant_call.cpp:1211
#15 Variant::callp (this=<optimized out>, p_method=..., p_args=0x7fffffffbe28, p_argcount=2, r_ret=..., r_error=...)
--Type <RET> for more, q to quit, c to continue without paging--
    at core/variant/variant_call.cpp:1196
#16 0x00000000007a50ad in GDScriptFunction::call (this=<optimized out>, p_instance=<optimized out>, p_args=<optimized out>, 
    p_argcount=<optimized out>, r_err=..., p_state=<optimized out>) at modules/gdscript/gdscript_vm.cpp:1750
#17 0x0000000000656296 in GDScriptInstance::callp (this=0x965ac00, p_method=..., p_args=0x7fffffffc1e8, p_argcount=1, r_error=...)
    at modules/gdscript/gdscript.cpp:2032
#18 0x0000000001325742 in Node::_gdvirtual__process_call<false> (this=0x96382b0, arg1=0.0083333333333333332)
    at core/variant/variant.cpp:2474
#19 Node::_notification (this=0x96382b0, p_notification=<optimized out>) at scene/main/node.cpp:55
#20 0x000000000058683d in Node::_notificationv (this=<optimized out>, p_notification=<optimized out>, p_reversed=<optimized out>, 
    this=<optimized out>, p_notification=<optimized out>, p_reversed=<optimized out>) at ./scene/main/node.h:50
#21 Node3D::_notificationv (this=0x96382b0, p_notification=17, p_reversed=<optimized out>) at ./scene/3d/node_3d.h:52
#22 0x00000000035b8773 in Object::notification(int, bool) [clone .constprop.0] (this=0x96382b0, p_notification=17, p_reversed=false)
    at core/object/object.cpp:870
#23 0x0000000001378bf7 in SceneTree::_process_group (this=0x95cab60, p_group=<optimized out>, p_physics=false)
    at scene/main/scene_tree.cpp:962
#24 0x0000000001392ac6 in SceneTree::_process (this=this@entry=0x95cab60, p_physics=p_physics@entry=false)
    at scene/main/scene_tree.cpp:1039
#25 0x0000000001393562 in SceneTree::process (this=0x95cab60, p_time=0.0083333333333333332) at scene/main/scene_tree.cpp:526
#26 0x00000000004fa505 in Main::iteration () at main/main.cpp:4111
#27 0x0000000000478cc1 in OS_LinuxBSD::run (this=0x7fffffffc140) at platform/linuxbsd/os_linuxbsd.cpp:962
#28 OS_LinuxBSD::run (this=this@entry=0x7fffffffc730) at platform/linuxbsd/os_linuxbsd.cpp:945
#29 0x0000000000443582 in main (argc=<optimized out>, argv=0x7fffffffcd78) at platform/linuxbsd/godot_linuxbsd.cpp:85

Footnotes

  1. No pathtraced reflections, unfortunately – only diffuse light.

@sourcelocation sourcelocation force-pushed the expose-lightmap-baking branch from a45fee6 to 1e6d769 Compare August 16, 2024 10:09
@sourcelocation
Copy link
Author

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
Copy link
Author

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 🙂

@ExplodingImplosion
Copy link

this looks awesome! keep up the good work.

@boredcoder411
Copy link

LGTM

@StefanH-AT

This comment was marked as off-topic.

@sourcelocation
Copy link
Author

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

@akien-mga
Copy link
Member

akien-mga commented Sep 19, 2024

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 👍 to the OP, and ideally, test the pull request and confirm that it does what you need in your projects.

@akien-mga akien-mga requested a review from a team September 19, 2024 11:18
@sinewavey
Copy link

sinewavey commented Sep 22, 2024

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

@sourcelocation
Copy link
Author

sourcelocation commented Sep 23, 2024

@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)?

@Calinou
Copy link
Member

Calinou commented Sep 23, 2024

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).

@sourcelocation
Copy link
Author

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.

@RisingThumb
Copy link

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

@NHodgesVFX
Copy link
Contributor

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

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