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] Batch of lightmapper fixes and minor improvements #46932

Merged
merged 1 commit into from
Mar 12, 2021

Conversation

JFonS
Copy link
Contributor

@JFonS JFonS commented Mar 12, 2021

  • Fix objects with no material being considered as fully transparent by the lightmapper. Fixes Baked Lightmap Regression #46861.
  • Added "environment_min_light" property: gives artistic control over the shadow color.
  • Fixed "Custom Color" environment mode, it was ignored before.
  • Added "interior" property to BakedLightmapData: controls whether dynamic capture objects receive environment light or not. Fixes CPU lightmapper capture doesn't make use of the environment lighting #46591.
  • Automatically update dynamic capture objects when the capture data changes (also works for "energy" which used to require object movement to trigger the update).
  • Added "use_in_baked_light" property to GridMap: controls whether the GridMap will be included in BakedLightmap bakes.
  • Set "flush zero" and "denormal zero" mode for SSE2 instructions in the Embree raycaster. According to Embree docs it should give a performance improvement.

@JFonS JFonS added this to the 3.2 milestone Mar 12, 2021
@JFonS JFonS requested review from a team as code owners March 12, 2021 10:00
@@ -33,6 +33,7 @@
// From Embree.
#include <math/vec2.h>
#include <math/vec3.h>
#include <xmmintrin.h>
Copy link
Member

Choose a reason for hiding this comment

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

Note that this won't compile on !SSE2 AFAIK. That's currently guarded in config.py since we only support x86_64 currently, but eventually this will need to be improved (not necessarily in this PR).

Copy link
Member

Choose a reason for hiding this comment

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

And it would probably crash if run on non-SSE capable CPU (e.g. 32 bit x86). I assume from above you don't build in the lightmapper into 32 bit builds, or something like this. Mind you you'd be hard pressed to find a non-SSE1 capable CPU these days lol.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah both Embree and OIDN only support x86_64 currently, so modules/denoise and modules/raycast are both only compiled on this arch. There's an aarch64 fork for Embree but we have yet to investigate using it (for macOS aarch64 mostly): https://github.com/lighttransport/embree-aarch64

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I'll be taking a look at embree-aarch64 since we will need it for 4.0's occlusion culling too. But in the meantime this change should not cause any issues given it's disabled on anything other than x86_64.

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.

Left some comments, looks good overall.

@JFonS JFonS force-pushed the fix_lm_capture_env branch from c16693b to 7b74564 Compare March 12, 2021 10:42
@JFonS
Copy link
Contributor Author

JFonS commented Mar 12, 2021

Pushed a new version, all the comments should be addressed now :)

- Fix objects with no material being considered as fully transparent by the lightmapper.
- Added "environment_min_light" property: gives artistic control over the shadow color.
- Fixed "Custom Color" environment mode, it was ignored before.
- Added "interior" property to BakedLightmapData: controls whether dynamic capture objects receive environment light or not.
- Automatically update dynamic capture objects when the capture data changes (also works for "energy" which used to require object movement to trigger the update).
- Added "use_in_baked_light" property to GridMap: controls whether the GridMap will be included in BakedLightmap bakes.
- Set "flush zero" and "denormal zero" mode for SSE2 instructions in the Embree raycaster. According to Embree docs it should give a performance improvement.
@JFonS JFonS force-pushed the fix_lm_capture_env branch from 7b74564 to e2c2867 Compare March 12, 2021 11:01
@akien-mga akien-mga merged commit 3f246eb into godotengine:3.2 Mar 12, 2021
@akien-mga
Copy link
Member

Thanks!

@Calinou
Copy link
Member

Calinou commented Mar 12, 2021

Added "environment_min_light" property: gives artistic control over the shadow color.

This isn't urgent for 3.2.4, but I was thinking about this since the "min_light" terminology reminds me a lot of the minlight functionality available in Quake lightmapping tools.

In particular, we may want to have an option to add the ambient lighting to the lightmap instead of clamping the darkest lighting values to avoid making ambient lighting too uniform. Improved versions of the Quake light tool (such as the one linked above) provide this as an -addmin option:

−addmin

Changes the behaviour of minlight. Instead of increasing low light levels to the global minimum, add the global minimum light level to all style 0 lightmaps. This may help reducing the sometimes uniform minlight effect.

Is this already supported by configuring ambient light in the Environment? If so, disregard this 🙂

@JFonS
Copy link
Contributor Author

JFonS commented Mar 12, 2021

@Calinou I was debating myself whether to add a "min" or an "add" option and I decided to go with "min" because it won't modify the overall brightness of the scene, it will only affect dark spots.

But I can see how a global "add" could also be useful. Right now you can set a custom environment color that adds light to the scene, but it takes geometry into account, so regions of the scene that are mostly covered wil not receive much of that light.

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.

4 participants