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

[3.2] New CPU lightmapper #44628

Merged
merged 7 commits into from
Jan 15, 2021
Merged

Conversation

JFonS
Copy link
Contributor

@JFonS JFonS commented Dec 23, 2020

Completely re-write the lightmap generation code:

  • Follow the general lightmapper code structure from 4.0.
  • Use proper path tracing to compute the global illumination.
  • Use atlassing to merge all lightmaps into a single texture (done by @RandomShaper)
  • Use OpenImageDenoiser to improve the generated lightmaps.

There is some minor compatibility breakage in some properties and methods
in BakedLightmap, but lightmaps generated in previous engine versions
should work fine out of the box.

The scene importer has been changed to generate .unwrap_cache files
next to the imported scene files. These files SHOULD be added to any
version control system as they guarantee there won't be differences when
re-importing the scene from other OSes or engine versions.

Documentation is missing, I'll work on it after the holidays :)

This work started as a Google Summer of Code project; Was later funded by IMVU for a good amount of progress;
Was then finished and polished by me on my free time.

TPS demo with direct light only:
lm-off

TPS demo with baked indirect lighting.
lm-on

Special thanks to Pedro (@RandomShaper) for helping out with atlassing and other issues!

Supersedes #40035.
This closes #15055, closes #16820, closes #16398, closes #20321, closes #21587, closes #30789, closes #30929, closes #32746, closes #38685, closes #39532.

Edit:
I updated the PR with fixes for all the issues reported so far:

@NHodgesVFX
Copy link
Contributor

NHodgesVFX commented Dec 23, 2020

The light mapper works really well with the exception of dynamic objects, I couldn't seem to get them to work properly.

rough bake times with 10 bounces, environment light on scene, denoiser on, and capture on high. 20 cores 40 Threads at 2.2ghz, boost 3.1ghz. quality is acceptable on low.

ultra: 30secs
high: 18secs
medium: 12secs
low: 8secs

Here is a image from blender's cycles engine
Cycles

Here is one in godot with this lightmapper, lights are set to indirect only.
Godot

here is with lights set to all
Godot_AllBaked

here is with no lights, the scene is lit with environment light only
Godot_EnviormentOnly

with no indirect light.
Godot_NoGI

with GIProbe
Godot_GIProbe

@NHodgesVFX
Copy link
Contributor

Here is the scene
LightMapperTest.zip

@dedm0zaj
Copy link

dedm0zaj commented Dec 24, 2020

In editor:

изображение

In game:

изображение

Project
test_lightmap_capture_energy_new.zip

upd:
Reimporting models helped

@dedm0zaj
Copy link

Test with my change (Energy parameter of DataLight affects dynamic objects)
#44458

gd_capture_energy_new.mp4

@JFonS
Copy link
Contributor Author

JFonS commented Dec 24, 2020

@NHodgesVFX Do you have a reproduction project for the dynamic objects issue? I'm testing and everything seems to work fine for me.

@JFonS
Copy link
Contributor Author

JFonS commented Dec 24, 2020

@dedm0zaj That's a separate issue, I will follow up on your PR soon.

@NHodgesVFX
Copy link
Contributor

@NHodgesVFX Do you have a reproduction project for the dynamic objects issue? I'm testing and everything seems to work fine for me.

i was trying to get it to work with the cornel's box scene above. This is what it looks like. Im wondering if its a scale issue. I just set the two small sphere to be dynamic. Here is the scene as it is in the photo
CornelBox_Dynamic.zip

godot dynamic

@NHodgesVFX
Copy link
Contributor

Update it does seem to be a scale issue, is it possible to get the cell size slider to go smaller? I scaled up the scene 5x and that made it work. Something I noticed is it might be nice to exaggerate the GI, although I would worry about that later.

@JFonS
Copy link
Contributor Author

JFonS commented Dec 24, 2020

@NHodgesVFX I set a lower bound to the cell size because the amount of stored data can grow pretty quickly, but it can be lowered.

As per the exaggeration, @dedm0zaj is working on a change so that the "Energy" property of BakedLightmapData also affects dynamics objects, so that should do fine.

@NHodgesVFX
Copy link
Contributor

@NHodgesVFX I set a lower bound to the cell size because the amount of stored data can grow pretty quickly, but it can be lowered.

As per the exaggeration, @dedm0zaj is working on a change so that the "Energy" property of BakedLightmapData also affects dynamics objects, so that should do fine.

I unbounded the values or at least i think i did, about to try it now.

@NHodgesVFX
Copy link
Contributor

Ok im at a loss, it doesn't work, the scene I listed above for whatever reason doesn't work. It does seem like the unbounded capture size worked fine though judging by bake time and size. Probably best if you give it a check when you have the chance. Although its Christmas now, so take a break :) Happy Holidays

@JFonS
Copy link
Contributor Author

JFonS commented Dec 24, 2020

@NHodgesVFX I will get back to this after the break then, happy holidays to you too!

@esblinuxcss
Copy link

Completely re-write the lightmap generation code:

  • Follow the general lightmapper code structure from 4.0.
  • Use proper path tracing to compute the global illumination.
  • Use atlassing to merge all lightmaps into a single texture (done by @RandomShaper)
  • Use OpenImageDenoiser to improve the generated lightmaps.

There is some minor compatibility breakage in some properties and methods
in BakedLightmap, but lightmaps generated in previous engine versions
should work fine out of the box.

The scene importer has been changed to generate .unwrap_cache files
next to the imported scene files. These files SHOULD be added to any
version control system as they guarantee there won't be differences when
re-importing the scene from other OSes or engine versions.

Documentation is missing, I'll work on it after the holidays :)

This work started as a Google Summer of Code project; Was later funded by IMVU for a good amount of progress;
Was then finished and polished by me on my free time.

TPS demo with direct light only:
lm-off

TPS demo with baked indirect lighting.
lm-on

Special thanks to Pedro (@RandomShaper) for helping out with atlassing and other issues!

Supersedes #40035.
This closes #15055, closes #16820, closes #16398, closes #20321, closes #21587, closes #30789, closes #30929, closes #32746, closes #38685, closes #39532.

I try to bake a small screne created with Qodot Plugin it take very longer time(3 and a half hours) to bake the scre with 4 Lights15 bounces in high quality my pc is not a power house is laptop with 1.66 Ghz Dual Core AMD CPU with 8Gb of RAM the current Ligthmapper takes 7-12minutes to bake the scne (using come trace and the ligths are set in Bake All) i will try to make another test with a more small scene created in blender with one ligth. when i do that i will show the results

@NHodgesVFX
Copy link
Contributor

NHodgesVFX commented Dec 26, 2020

@esblinuxcss this is expected, the cone tracing mode of the previous light mapper was a fast but inaccurate mode based off of GiProbe. The new lightmapper uses a much better version of the raytrace mode of the previous lightmapper. In addition you are using 15 bounces the previous lightmapper only used 1, more bounces more time. Also Highquality mode takes more time,you should be able to get an acceptable result with low. Try it on low with 3 bounces.

Make sure you are building with target=release_debug or you are downloading an artifact from here which is compiled using release_debug.

@Calinou
Copy link
Member

Calinou commented Dec 26, 2020

@esblinuxcss We could consider adding an easy way to bake with a "preview" quality before committing to a "final" bake. This way, you could keep your current settings in your BakedLightmap node, yet you'd have the ability to perform a quick bake with low quality settings.

I opened a proposal to discuss this: godotengine/godot-proposals#2029

@Avogine
Copy link

Avogine commented Dec 29, 2020

I've got a question regarding draw calls. When using baked light, it seems like every object with baked light is adding 1 extra draw call. How are you supposed to optimize a forest with hundreds of trees, for example, where you would normally use multimeshes?

Comment on lines +36 to +38
#define STB_RECT_PACK_IMPLEMENTATION
#include "thirdparty/stb_rect_pack/stb_rect_pack.h"
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick as it's preexisting code, but please add a separation line between Godot headers ("core/print_string.h") and thirdparty/system headers.

@reduz
Copy link
Member

reduz commented Dec 29, 2020

@Avogine you just don't use lightmaps for such kind of scenes. For outdoors, you will have to wait for Godot 4 SDFGI, or just not use them.

@Avogine
Copy link

Avogine commented Dec 29, 2020

Ok thanks, good to know

@@ -0,0 +1,629 @@
// stb_rect_pack.h - v1.00 - public domain - rectangle packing
Copy link
Member

@akien-mga akien-mga Dec 29, 2020

Choose a reason for hiding this comment

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

This can go in thirdparty/misc/ like other stb headers, and like done in master.

Also make sure to document it in thirdparty/README.md (under "Misc / Modules", you can copy from stb_vorbis.c and adjust).
Edit: That's done already, I had missed the file.

Comment on lines +31 to +35
#include "denoise_wrapper.h"
#include "core/os/copymem.h"
#include "core/os/memory.h"
#include "thirdparty/oidn/include/OpenImageDenoise/oidn.h"
#include <stdio.h>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
#include "denoise_wrapper.h"
#include "core/os/copymem.h"
#include "core/os/memory.h"
#include "thirdparty/oidn/include/OpenImageDenoise/oidn.h"
#include <stdio.h>
#include "denoise_wrapper.h"
#include "core/os/copymem.h"
#include "core/os/memory.h"
#include "thirdparty/oidn/include/OpenImageDenoise/oidn.h"
#include <stdio.h>

See https://docs.godotengine.org/en/latest/community/contributing/code_style_guidelines.html#header-includes

Note that normally you shouldn't include thirdparty headers with a fully qualified path like this, it would be better to add them to the include path and use #include <OpenImageDenoise/oidn.h>. But given that it's done the same way in master, no need to change it there. One day I'll get to allow unbundling this dependency on Linux distros and that will be part of it).

@JFonS JFonS force-pushed the new_cpu_lightmapper_3.2 branch 3 times, most recently from d85b315 to ff40fa3 Compare January 14, 2021 18:08
@JFonS
Copy link
Contributor Author

JFonS commented Jan 14, 2021

I updated the PR with fixes for all the issues reported so far:

@clayjohn
Copy link
Member

Changes to shader compiler look good! I havent reviewed anything else, but I trust the above reviews are sufficient in that regard.

JFonS added 2 commits January 15, 2021 12:31
- Fix Embree runtime when using MinGW (patch by @RandomShaper).
- Fix baking of lightmaps on GridMaps.
- Fix some GLSL errors.
- Fix overflow in the number of shader variants (GLES2).
@JFonS JFonS force-pushed the new_cpu_lightmapper_3.2 branch from ff40fa3 to b1ca82c Compare January 15, 2021 11:34
Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Let's get this merged :)

@akien-mga akien-mga merged commit 497653a into godotengine:3.2 Jan 15, 2021
@akien-mga
Copy link
Member

Thanks for the amazing work!

@Jummit
Copy link
Contributor

Jummit commented Jan 16, 2021

@JFonS You forgot to add documentation for the new settings. Most of them are pretty obvious, but I bet there are some things that are good to know, but only you are aware of.

Also, is there a reason why max_atlas_size has a minimum value of 2053?

@Calinou
Copy link
Member

Calinou commented Jan 17, 2021

@Jummit Doesn't #44628 cover the documentation?

@Jummit
Copy link
Contributor

Jummit commented Jan 17, 2021

@Jummit Doesn't #44628 cover the documentation?

Weird, it looks like it, but they don't appear in the documentation in my build. Probably a mistake on my end.

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.