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

Modify display and documentation of attenuation for Light3D #87583

Merged
merged 1 commit into from
Feb 8, 2024

Conversation

milkiq
Copy link
Contributor

@milkiq milkiq commented Jan 25, 2024

When using OmniLight3D or SpotLight3D in a 3D scene, adjusting the attenuation property doesn't behave as expected in the editing panel. The default value of 1.0 doesn't result in linear changes in light intensity over distance, and the modes selected through right-clicking on the property, such as Zero and Ease In-Out, don't influence the light intensity decay as their names suggest.

This description can be confusing for users. For example, with Attenuation set to 1, Range set to 5, and the light source positioned at (0, 2.5, 0), the color values at the center point of a plane with no lighting (color: rgb(0.5,0.5,0.5)) and a default white plane at (0,0,0) (color: rgb(1,1,1)) are not the same. It is evident that the decay is not linear.

PixPin_2024-01-25_20-08-36

Upon inspecting the source code, found that Godot uses the following formula to calculate light attenuation in Forward+ mode:

float get_omni_attenuation(float distance, float inv_range, float decay) {
    float nd = distance * inv_range;
    nd *= nd;
    nd *= nd; // nd^4
    nd = max(1.0 - nd, 0.0);
    nd *= nd; // nd^2
    return nd * pow(max(distance, 0.0001), -decay);
}

Here, distance is the distance from the light to the vertex, inv_range is the reciprocal of the light's range property, and decay is the light's attenuation property.

The formula can be expressed as:

$$ attenuation = \frac{(1 - (\frac{distance}{range})^4)^2}{distance^{decay}} $$

When attenuation is 1 and range is 5, the distance decay function produces the following curve:

image-20240125201324384

Clearly, it is not linear within the range [0, 5].

Conversely, when attenuation is 0.5 and range is 5, it behaves more like linear decay:

image-20240125201912582

In the editor, the attenuation property is displayed using an ease curve:

ADD_PROPERTYI(PropertyInfo(Variant::FLOAT, "omni_attenuation", PROPERTY_HINT_EXP_EASING, "attenuation"), "set_param", "get_param", PARAM_ATTENUATION);

The curve code is:

double Math::ease(double p_x, double p_c) {
	if (p_x < 0) {
		p_x = 0;
	} else if (p_x > 1.0) {
		p_x = 1.0;
	}
	if (p_c > 0) {
		if (p_c < 1.0) {
			return 1.0 - Math::pow(1.0 - p_x, 1.0 / p_c);
		} else {
			return Math::pow(p_x, p_c);
		}
	} else if (p_c < 0) {
		//inout ease

		if (p_x < 0.5) {
			return Math::pow(p_x * 2.0, -p_c) * 0.5;
		} else {
			return (1.0 - Math::pow(1.0 - (p_x - 0.5) * 2.0, -p_c)) * 0.5 + 0.5;
		}
	} else {
		return 0; // no ease (raw)
	}
}

This curve code clearly differs from the actual distance attenuation function. Therefore, it is suggested to replace PROPERTY_HINT_EXP_EASING in the editor with PROPERTY_HINT_RANGE and set the default value to 0. When the default value is 0, the light attenuation is more uniform within the range, and adjusting the range from 5 to 10 significantly increases the coverage:

image-20240125202637904

When set to 1, adjusting the range has almost no effect on the function, and beyond half the range, the light's impact on objects is minimal (even with a range set to 4096, objects beyond 20 units from the light receive less than 1/20th of the illumination), which might not align with the developer's expectation when adjusting the range to cover a larger area:

image-20240125202817845

The issue with SpotLight3D is similar to OmniLight3D and should be addressed accordingly.

@AThousandShips
Copy link
Member

AThousandShips commented Jan 25, 2024

Changing the default value breaks compatibility, it shouldn't be changed unless it fixes some bug (can't see this from the description, if the documentation is wrong it should be changed, not the default, as people who pick this original value still want it, and it'll now be different in all their projects)

Default values are not stored in scenes, so anyone who picked 1.0 will now suddenly have 0.0, drastically different

@LiBooks
Copy link

LiBooks commented Jan 25, 2024

Changing the default value breaks compatibility, it shouldn't be changed unless it fixes some bug (can't see this from the description, if the documentation is wrong it should be changed, not the default, as people who pick this original value still want it, and it'll now be different in all their projects)

Default values are not stored in scenes, so anyone who picked 1.0 will now suddenly have 0.0, drastically different

But I believe that it is very necessary to change this default value in the future. The value provided by the default should be applicable to common operations, but under the current default value, even if the range of a point light source is set to 4000 meters, its effective illumination range is less than 20 meters. This is a serious error when combined with the incorrect ease curve provided in the editor. It has misled the development of many engine functions and 3D demos in the past few years.

@AThousandShips
Copy link
Member

That's for the future though, and needs to be discussed in a proposal I'd say, currently it breaks compatibility, it being ill suited isn't a reason to break compatibility

It has misled the development of many engine functions and 3D demos in the past few years.

Can you provide a source for this claim?

@LiBooks
Copy link

LiBooks commented Jan 25, 2024

That's for the future though, and needs to be discussed in a proposal I'd say, currently it breaks compatibility, it being ill suited isn't a reason to break compatibility

It has misled the development of many engine functions and 3D demos in the past few years.

Can you provide a source for this claim?

You can test it yourself by creating a point light source and a 3D mesh object. Then drag the light range, and you will find the situation I mentioned. In the past year, about three game developers have mentioned to me the problem of point light source anomalies when creating indoor scenes. They developed based on the attenuation curve displayed in the editor during development, but they could not create an effective point light source that illuminates a wide indoor area. The curve is seriously inconsistent with the actual lighting situation. And when the default value is 1, the light will decay rapidly with distance, which is also inconsistent with the development ideas of most game developers.

@AThousandShips
Copy link
Member

That's hearsay and anecdotal evidence though 🙃 changing the default value is only really valid when the old value is completely useless and it's certain essentially no one is using it or that no one should be using it

Changing this value needs more than that, I'd suggest opening a proposal to discuss this and keep it out of this PR

@LiBooks

This comment was marked as duplicate.

@milkiq
Copy link
Contributor Author

milkiq commented Jan 25, 2024

So, in this PR, we just modify its display, replacing the ease curve with a range value, and updated the documentation to demonstrate how adjusting attenuation can better suit the user's project. Correct?

Changing this value needs more than that, I'd suggest opening a proposal to discuss this and keep it out of this PR

By the way, if we open a proposal to discuss the distance attenuation function, it might be beneficial to compare how other game engines control light attenuation. In previous attenuation functions, attempting to fit physical lighting effects may not be the best approach. It is more important to enable users to understand and achieve the desired lighting effects conveniently.

@AThousandShips
Copy link
Member

AThousandShips commented Jan 25, 2024

So, in this PR, we just modify its display, replacing the ease curve with a range value, and updated the documentation to demonstrate how adjusting attenuation can better suit the user's project. Correct?

I'd say so yes

If the change to the display doesn't work well with the original default the whole change needs to be discussed further to see if it's justified or can be done differently to avoid taht

@milkiq milkiq requested a review from a team as a code owner January 25, 2024 15:55
@milkiq
Copy link
Contributor Author

milkiq commented Jan 25, 2024

Changed commit.

@clayjohn
Copy link
Member

Just jumping in to add some context here. We use a physically accurate inverse square falloff in Godot with a modification to make the intensity drop off to 0 at the end of the range. The attenuation property allows you to tweak the falloff if you do not want physically accurate lighting.

An attenuation value of 1 does not and should not indicate a linear falloff. It indicates that no change is made to the default inverse square falloff.

I think its a good idea to focus our conversation more on what users will find intuitive for editing.

By the way, if we open a proposal to discuss the distance attenuation function, it might be beneficial to compare how other game engines control light attenuation. In previous attenuation functions, attempting to fit physical lighting effects may not be the best approach. It is more important to enable users to understand and achieve the desired lighting effects conveniently.

In Godot 3.x, we didn't use the physically-based attenuation by default. So the default curve was the linear falloff (attenuation = 1.0) and it was really confusing for users. They had a extremely hard time to make lights look good and they complained a lot. We changed the default in Godot 4.0 and until now, nobody has complained or even noticed, instead we have a lot of people exclaiming how good lights look by default now.

As far as I know, all the other major engines have been using a formula similar to ours for many years now.

I think you are probably right and what is confusing is the fact that attenuation is displayed as a curve which seems to indicate that it is the only thing used to calculate the falloff of the light source.

@milkiq
Copy link
Contributor Author

milkiq commented Jan 26, 2024

I've revised the documentation based on your suggestion. @Mickeon

@milkiq milkiq changed the title Modify display and default value of attenuation for Light3D Modify display and documentation of attenuation for Light3D Jan 26, 2024
@LiBooks
Copy link

LiBooks commented Jan 26, 2024

Just jumping in to add some context here. We use a physically accurate inverse square falloff in Godot with a modification to make the intensity drop off to 0 at the end of the range. The attenuation property allows you to tweak the falloff if you do not want physically accurate lighting.

An attenuation value of 1 does not and should not indicate a linear falloff. It indicates that no change is made to the default inverse square falloff.

I think its a good idea to focus our conversation more on what users will find intuitive for editing.

By the way, if we open a proposal to discuss the distance attenuation function, it might be beneficial to compare how other game engines control light attenuation. In previous attenuation functions, attempting to fit physical lighting effects may not be the best approach. It is more important to enable users to understand and achieve the desired lighting effects conveniently.

In Godot 3.x, we didn't use the physically-based attenuation by default. So the default curve was the linear falloff (attenuation = 1.0) and it was really confusing for users. They had a extremely hard time to make lights look good and they complained a lot. We changed the default in Godot 4.0 and until now, nobody has complained or even noticed, instead we have a lot of people exclaiming how good lights look by default now.

As far as I know, all the other major engines have been using a formula similar to ours for many years now.

I think you are probably right and what is confusing is the fact that attenuation is displayed as a curve which seems to indicate that it is the only thing used to calculate the falloff of the light source.

Thank you very much for your response. In addition, I would like to mention that currently Godot uses a physical formula for attenuation by default, but the light intensity is measured in non-physical units by default. Therefore, I suggest that the formula used and the units of measurement be unified by default. Thank you again for your response.

doc/classes/OmniLight3D.xml Outdated Show resolved Hide resolved
doc/classes/OmniLight3D.xml Outdated Show resolved Hide resolved
doc/classes/SpotLight3D.xml Outdated Show resolved Hide resolved
doc/classes/SpotLight3D.xml Outdated Show resolved Hide resolved
doc/classes/OmniLight3D.xml Outdated Show resolved Hide resolved
doc/classes/SpotLight3D.xml Outdated Show resolved Hide resolved
@milkiq
Copy link
Contributor Author

milkiq commented Jan 26, 2024

You are right👍, I resolved them. @AThousandShips

@Mickeon
Copy link
Contributor

Mickeon commented Jan 26, 2024

I feel bad to nitpick but I feel the need to again, even though I have already gave my two cents on this PR.

@milkiq
Copy link
Contributor Author

milkiq commented Jan 29, 2024

Thanks for mentioning me. Good stuff, it just may need a few more tweaks.

Thanks for your advice, I resolved them all.

Copy link
Contributor

@Mickeon Mickeon left a comment

Choose a reason for hiding this comment

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

Documentation looks great, I cannot speak for the PROPERTY_HINT change

scene/3d/light_3d.cpp Outdated Show resolved Hide resolved
scene/3d/light_3d.cpp Outdated Show resolved Hide resolved
doc/classes/OmniLight3D.xml Outdated Show resolved Hide resolved
doc/classes/SpotLight3D.xml Outdated Show resolved Hide resolved
@milkiq
Copy link
Contributor Author

milkiq commented Jan 30, 2024

OK. Resolved all.

@milkiq milkiq requested review from Calinou and clayjohn February 1, 2024 09:27
@milkiq
Copy link
Contributor Author

milkiq commented Feb 4, 2024

@AThousandShips @clayjohn @Calinou Is there anything else I need to improve for this PR?

Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

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

Tested locally, it works as expected. As a bonus, this also makes it easier to select zero/negative attenuation values without having to right-click the property.

Testing project: test_pr_87583.zip

Before

omni_before.mp4
spot_before.mp4

After

omni_after.mp4
spot_after.mp4

doc/classes/SpotLight3D.xml Outdated Show resolved Hide resolved
doc/classes/OmniLight3D.xml Outdated Show resolved Hide resolved
doc/classes/OmniLight3D.xml Outdated Show resolved Hide resolved
doc/classes/SpotLight3D.xml Outdated Show resolved Hide resolved
@milkiq milkiq force-pushed the master branch 2 times, most recently from 77c1370 to 9655e69 Compare February 6, 2024 04:39
@akien-mga akien-mga merged commit 9484a37 into godotengine:master Feb 8, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks! And congrats for your first merged Godot 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.

7 participants