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

Validate physical light units in GI classes. #65807

Merged
merged 1 commit into from
Sep 29, 2022

Conversation

clayjohn
Copy link
Member

Add checks for physical light units in a few more places to ensure they aren't used when disabled.

Fixes: #65586

Add checks for physical light units in a few more places to ensure they aren't used when disabled.
@clayjohn clayjohn added this to the 4.0 milestone Sep 15, 2022
@clayjohn clayjohn requested a review from a team as a code owner September 15, 2022 00:41
@@ -1001,10 +1001,16 @@ LightmapGI::BakeError LightmapGI::bake(Node *p_from_node, String p_image_data_pa
lightmapper->add_directional_light(light->get_bake_mode() == Light3D::BAKE_STATIC, -xf.basis.get_column(Vector3::AXIS_Z).normalized(), linear_color, energy, l->get_param(Light3D::PARAM_SIZE), l->get_param(Light3D::PARAM_SHADOW_BLUR));
} else if (Object::cast_to<OmniLight3D>(light)) {
OmniLight3D *l = Object::cast_to<OmniLight3D>(light);
lightmapper->add_omni_light(light->get_bake_mode() == Light3D::BAKE_STATIC, xf.origin, linear_color, energy * (1.0 / (Math_PI * 4.0)), l->get_param(Light3D::PARAM_RANGE), l->get_param(Light3D::PARAM_ATTENUATION), l->get_param(Light3D::PARAM_SIZE), l->get_param(Light3D::PARAM_SHADOW_BLUR));
if (GLOBAL_GET("rendering/lights_and_shadows/use_physical_light_units")) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do I read this change correctly and was physical lighting the default before? Or should there be a calculation applied to energy when physical lighting is turned off?

Copy link
Member Author

Choose a reason for hiding this comment

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

There should not be a calculation applied when physical lighting is turned off. I accidently left the unit conversion in when making the original PR. But the unit conversion is only applicable when using physical light units. This bug resulted in OmniLights appearing too dark in baked lighting when PLU is turned off.

@BastiaanOlij
Copy link
Contributor

We're already doing this everywhere but I wonder if there is a better way to access our setting then copying the GLOBAL_GET("rendering/lights_and_shadows/use_physical_light_units") code everywhere. I was always told to prevent this and instead have a method that can be called to retrieve the value and retrieve it in one place. No idea if that is practical here..

Copy link
Contributor

@BastiaanOlij BastiaanOlij left a comment

Choose a reason for hiding this comment

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

I don't know if the calculation is what it should be but code wise this looks fine to me to merge.

@akien-mga akien-mga merged commit de31edb into godotengine:master Sep 29, 2022
@akien-mga
Copy link
Member

Thanks!

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.

Vulkan: Broken colors when baking LightmapGI
3 participants