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

Fix light intensity and attenuation import from GLTF #62747

Merged
merged 1 commit into from
Jul 5, 2022

Conversation

PZerua
Copy link
Contributor

@PZerua PZerua commented Jul 5, 2022

When importing a GLTF scene, Omni and Spot lights calculated an Attenuation factor that could be set to infinity when the Intensity was 0. Also the intensity was never directly stored.

As discussed in rocket-chat, the Attenuation is used for artistic control, so lets just default it to 1 and use the Intensity properly instead.

Please note, the user can still set the attenuation to a big value and generate infinities in this line of the light shader, so a proper range should be added too somehow. I think PROPERTY_HINT_EXP_EASING here can't have a min and max, so maybe we can change it to PROPERTY_HINT_RANGE?

Noticed after loading the new Sponza scene where lights are stored with intensity 0:

Before (intensity = 1, attenuation = inf)
before

After (intensity = 0, attenuation = 1)
after

@PZerua PZerua requested a review from a team as a code owner July 5, 2022 18:18
@fire fire requested a review from a team July 5, 2022 19:21
@fire
Copy link
Member

fire commented Jul 5, 2022

Approved the workflow.

@Calinou
Copy link
Member

Calinou commented Jul 5, 2022

Note that lights with energy set to 0 are still rendered by Godot, so they will incur a performance cost. If this is not desired, the light should have its visible property set to false.

Copy link
Member

@fire fire left a comment

Choose a reason for hiding this comment

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

I downloaded the intel sponza scene and tested that the light / scene is correct. Changing the energy changes the light and 0 is ambient (not black).

@akien-mga akien-mga added the cherrypick:3.x Considered for cherry-picking into a future 3.x release label Jul 5, 2022
@akien-mga akien-mga merged commit 1b057e1 into godotengine:master Jul 5, 2022
@akien-mga
Copy link
Member

Thanks! And congrats for your first merged Godot contribution 🎉

@akien-mga
Copy link
Member

Cherry-picked for 3.5.

@akien-mga akien-mga removed the cherrypick:3.x Considered for cherry-picking into a future 3.x release label Jul 6, 2022
@Calinou
Copy link
Member

Calinou commented Jul 6, 2022

Cherry-picked for 3.5.

I assume this fixes #54596 then.

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