Skip to content

Conversation

@ThatRedox
Copy link
Member

If Draw sun and `Sun Sampling Strategy are disabled, the sun still contributes to reflections:
image
(500 exposure).

This PR adds the drawSun check to Sun.diffuseIntersect which was causing this issue.

@ThatRedox ThatRedox added bug renderer snapshot-only Issues that only affect Chunky snapshots labels Apr 14, 2023
Copy link
Member

@Peregrine05 Peregrine05 left a comment

Choose a reason for hiding this comment

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

This is not a bug. Sunlight is meant to be disabled not with the Draw sun control, but rather with the Sun Sampling Strategy control set to NON_LUMINOUS.

This PR makes non-sampled sunlight again depend on the state of the Draw sun control (The issue was #1317, and was fixed by #1474). The Draw sun control should have no effect over the sunlight that the scene receives. If a user wishes to disable sunlight, then the user should set Sun Sampling Strategy to NON_LUMINOUS. Having more than one way to disable sunlight could be confusing to users.

@Peregrine05 Peregrine05 removed the bug label Apr 14, 2023
}

return false;
return doIntersect(ray, apparentTextureBrightness);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return doIntersect(ray, apparentTextureBrightness);
return doIntersect(ray, apparentTextureBrightness, false);

*/
public boolean intersectDiffuse(Ray ray) {
if (ray.d.dot(sw) < .5) {
return doIntersect(ray, color);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return doIntersect(ray, color);
return doIntersect(ray, color, true);

return doIntersect(ray, color);
}

private boolean doIntersect(Ray ray, Vector3 color) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
private boolean doIntersect(Ray ray, Vector3 color) {
private boolean doIntersect(Ray ray, Vector3 color, boolean isDiffuse) {

}

private boolean doIntersect(Ray ray, Vector3 color) {
if (!drawTexture || ray.d.dot(sw) < .5) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (!drawTexture || ray.d.dot(sw) < .5) {
if ((!isDiffuse && !drawTexture) || ray.d.dot(sw) < .5) {

@ThatRedox
Copy link
Member Author

This is not a bug. Sunlight is meant to be disabled not with the Draw sun control, but rather with the Sun Sampling Strategy control set to NON_LUMINOUS.

Calling it Sun Sampling Strategy is confusing then. And one would expect OFF to be able to turn off the sun, not NON_LUMINOUS.

@Peregrine05
Copy link
Member

I suggest that we put a hint in the Lighting tab then, as is done in other parts of the GUI.

The 'Draw sun' control only changes the visibility of the texture of the sun. Set 'Sun Sampling Strategy' to 'NON_LUMINOUS' to disable sunlight.

@ThatRedox ThatRedox marked this pull request as draft April 14, 2023 18:55
@ThatRedox
Copy link
Member Author

Replaced by #1577

@ThatRedox ThatRedox closed this Apr 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

renderer snapshot-only Issues that only affect Chunky snapshots

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants