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

[CPU Lightmapper] Account for ambient light settings when baking lights #62260

Merged
merged 1 commit into from
Jun 23, 2022

Conversation

needleful
Copy link
Contributor

This helps to address issue #56078, by checking for ambient light settings when the baked lightmap's using the "Scene" environment setting.
In my own game, this appears to match the environment lighting exactly.

@needleful needleful requested a review from a team as a code owner June 20, 2022 23:23
@clayjohn clayjohn requested a review from JFonS June 21, 2022 06:09
@clayjohn clayjohn added this to the 3.x milestone Jun 21, 2022
@JFonS
Copy link
Contributor

JFonS commented Jun 21, 2022

Looks great!

There's an issue with code formatting and CI is failing. You should run clang-format on your changes. You can read more about code style here. I recommend setting up the pre-commit hooks, so you automatically run all the style checks locally before every commit. All this is also explained in the docs.

Other than that, the two commits should be squashed into one. Here's a bit of documentation on why we prefer single commits and how to squash multiple commits into one by using interactive rebase.

@needleful needleful force-pushed the baked-lightmap-env-settings branch from 7d7f894 to eb99c72 Compare June 21, 2022 19:13
@needleful
Copy link
Contributor Author

Thanks. I'm not too familiar with the workflow. I've applied clang-format now. Hopefully it looks good

Copy link
Contributor

@JFonS JFonS left a comment

Choose a reason for hiding this comment

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

Looks great.

@JFonS JFonS merged commit c580f7c into godotengine:3.x Jun 23, 2022
@JFonS
Copy link
Contributor

JFonS commented Jun 23, 2022

Merged! Thanks for your contribution :)

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