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] Lightness of dynamic objects from Energy BakedLightmapData #44458

Merged

Conversation

dedm0zaj
Copy link

@dedm0zaj dedm0zaj commented Dec 17, 2020

Added the effect of Energy BakedLightmapData on dynamic objects.

image
image

On the GIF: cubes are dynamic objects.

gd_capture_energy

Works well in a running game.
In the editor, for the changes to the Energy parameter to take effect, you need to turn off and on the visibility of BakedLightmap.

image

Bugsquad edit: Closes godotengine/godot-proposals#1952.

@akien-mga akien-mga added this to the 3.2 milestone Dec 17, 2020
@dedm0zaj dedm0zaj changed the title Lightness of dynamic objects from Energy BakedLightmapData [3.2] Lightness of dynamic objects from Energy BakedLightmapData Dec 18, 2020
Copy link
Member

@clayjohn clayjohn left a comment

Choose a reason for hiding this comment

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

Looks reasonable to me. But I am unfamiliar with this code. Will need to wait for @JFonS to approve.

@JFonS
Copy link
Contributor

JFonS commented Dec 24, 2020

With this approach we need to make sure _update_instance_lightmap_captures is called whenever the energy is changed. Otherwise we can have instances that don't update properly.

I think a better alternative is to multiply the lightmap energy directly in the shader. That would be in:

if (lightmap_capture_sky) {
ambient_light = mix(ambient_light, captured.rgb, captured.a);
} else {
ambient_light = captured.rgb;
}
}

and for GLES2:
if (lightmap_capture_sky) {
ambient_light = mix(ambient_light, captured.rgb, captured.a);
} else {
ambient_light = captured.rgb;
}
}

The lightmap_energy uniform is already available, so there should be no update issues.

@dedm0zaj
Copy link
Author

@JFonS
I tried to do through shaders, but i am having problems:

  1. In scene.glsl, the lightmap_energy variable refers to the #ifdef USE_LIGHTMAP section.
    And it must be applied in the #ifdef USE_LIGHTMAP_CAPTURE section.
    I didn't understand how to do it.
  2. I created my capture_energy variable for the #ifdef USE_LIGHTMAP_CAPTURE section. And I tried to convey its meaning here:
    } else if (!e->instance->lightmap_capture_data.empty()) {
    glUniform4fv(state.scene_shader.get_uniform_location(SceneShaderGLES3::LIGHTMAP_CAPTURES), 12, (const GLfloat *)e->instance->lightmap_capture_data.ptr());
    state.scene_shader.set_uniform(SceneShaderGLES3::LIGHTMAP_CAPTURE_SKY, false);

    For this, I added a few lines:
RasterizerStorageGLES3::LightmapCapture *capture = storage->lightmap_capture_data_owner.getornull(e->instance->lightmap_capture->base);
state.scene_shader.set_uniform(SceneShaderGLES3::CAPTURE_ENERGY, capture->energy);

but e->instance->lightmap_capture is NULL

I still haven't found a way to do it through the shader.

@JFonS
Copy link
Contributor

JFonS commented Dec 24, 2020

ah right, my bad. I haven't looked deeply into it, but it seems like it won't be easy to get the capture energy when setting up the shader uniforms. So we're back to your implementation :)

I say we merge this PR and, if we find any case where capture is not updated after changing the energy, we can look into it.

@dedm0zaj
Copy link
Author

@JFonS
There is an update problem in the editor. To update, you need to turn off and on the visibility of BakedLightmap.
I mentioned this in the topic.

If the change is done in the shader, then this problem will not occur.

@dedm0zaj dedm0zaj requested a review from JFonS December 24, 2020 17:08
@dedm0zaj
Copy link
Author

@JFonS oops! I accidentally pressed "Re-request review". I don't know much about github and its functions yet. Sorry

@JFonS
Copy link
Contributor

JFonS commented Dec 24, 2020

@dedm0zaj No worries :)

@akien-mga akien-mga merged commit 4436f95 into godotengine:3.2 Dec 25, 2020
@akien-mga
Copy link
Member

Thanks!

@akien-mga akien-mga modified the milestones: 3.2, 3.3 Apr 21, 2021
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