Page MenuHome

[WIP] Sky Texture fixes
Needs ReviewPublic

Authored by Marco (nacioss) on Mon, Jun 22, 2:44 PM.

Details

Reviewers
Lukas Stockner (lukasstockner97)
Group Reviewers
Cycles
Summary

This patch brings some fixes and UI changes to the Nishita Sky Texture:

  • Add Sun Intensity parameter:

again thanks to users feedback, this adds a new parameter to control the intensity of the sun disc for better control of the sun lighting;

  • Remove limit for Sun Rotation parameter:

thanks to users feedback, this change is helpful for example to control the sun rotation via drivers;

  • Remove Vector input:

this was a necessary change due to the fact that the MIS samples the sun position based only on the Sun Elevation and Rotation parameters and because we did a "hack" for all the other cases (such as connected Mapping Node) to set the MIS map to 4k internally;

  • Change Altitude parameter from meters to kilometers:

having the slider in meters made it hard for people to change the altitude really,;

  • Sample more pixels toward the horizon:

given the fact that the sky has more visible color changes near the horizon, then sampling more pixels toward it produces better results (this doesn't affect performance because less pixels are sampled toward zenith and so the number of precomputed pixels is the same really);

  • Fix sun issue at Altitude 0:

the problem was given because the rays shoot by the virtual camera that samples the precomputed texture could intersect with the earth surface giving artifacts such as https://developer.blender.org/T78032. Shifting the altitude internally a bit solved the issue;

  • Fix: old files saved with older sky methods:

projects saved with versions before 2.90 got 0 as parameters for Nishita method

Patch is still in WIP...

Diff Detail

Repository
rB Blender
Branch
sky-fixes
Build Status
Buildable 8821
Build 8821: arc lint + arc unit

Event Timeline

Marco (nacioss) requested review of this revision.Mon, Jun 22, 2:44 PM
Marco (nacioss) created this revision.
Marco (nacioss) retitled this revision from sample more pixels toward the horizon, fix earth intersection issue, Altitude slider in km to Sky Texture fixes.Mon, Jun 22, 2:45 PM
Marco (nacioss) edited the summary of this revision. (Show Details)
Marco (nacioss) edited the summary of this revision. (Show Details)Mon, Jun 22, 2:49 PM

missed to update the OSL part

Thanks for the patch!

There are couple of things I'm missing here.

First is the atomic nature of changes. If changes are not dependent on each other they should happen in separate commits. This helps on multiple levels:

  • Review becomes way easier and faster
  • Helps with presentation to users
  • Helps finding source of possible regression more easily

Second thing is more explanation of what the changes bring:

  • What was the exact issue with 0 altitude?
  • How the more pixels sampled towards horizon affect performance and image quality?
  • Does the change form meters to kilometers break compatibility of existing files?
Marco (nacioss) edited the summary of this revision. (Show Details)Fri, Jun 26, 12:20 PM

There are couple of things I'm missing here.

First is the atomic nature of changes. If changes are not dependent on each other they should happen in separate commits. This helps on multiple levels:

  • Review becomes way easier and faster
  • Helps with presentation to users
  • Helps finding source of possible regression more easily

Sorry everyone, i didn't specify the patch to be still in WIP since i'm waiting for a change by @Lukas Stockner (lukasstockner97) .
The thing really is that i don't have commit rights yet, and so created this gigantic patch full of changes for the Nishita Sky Texture.
Anyway i just updated the description of the patch with the missing informations, thanks for pointing that out.

  • code cleanup
  • remove Vector input from the UI
  • fix: old files saved with older sky methods got 0 as parameters for Nishita
Marco (nacioss) edited the summary of this revision. (Show Details)Sat, Jun 27, 4:58 PM
Ankit (ankitm) retitled this revision from Sky Texture fixes to [WIP] Sky Texture fixes.Sat, Jun 27, 6:00 PM
Lukas Stockner (lukasstockner97) requested changes to this revision.Thu, Jul 2, 12:58 AM

First pass of review, I'll look into handling the mapping settings for the sun sampling now.

As for the separate patches: You can just open multiple patches, it's no problem to commit them separately
Maybe keep stuff that changes the same areas of the code together (like the altitude clipping and meter->kilometer change), but e.g. the versioning and the socket hiding can definitely be split into separate patches.

Regarding compatibility for altitude: That can be handled in versioning code by bumping the subversion.

intern/cycles/kernel/shaders/node_sky_texture.osl
176

Please use sqrt() here, pow() is very expensive so we should avoid it if possible.

intern/cycles/kernel/svm/svm_sky.h
183

Same here (safe_sqrtf outside of OSL).

intern/cycles/render/nodes.cpp
763–768

Hm, this seems unnecessarily complicated. I think something based on fmodf() should be cleaner.

intern/cycles/util/util_sky_nishita.cpp
300

As above, please prefer sqr() over pow() here.

This revision now requires changes to proceed.Thu, Jul 2, 12:58 AM
  • Cleanup: Lukas changes
Marco (nacioss) marked 4 inline comments as done.Thu, Jul 2, 9:41 AM

I would prefer to maintain this patch full of all the changes as i don't have commits access yet, probably in the future will make separate fixes as it should be, but again i need commit rights for that.
Said that, i'm not sure how to do the Altitude bump.
Also there is this bug that needs to be fixed: https://developer.blender.org/T78324

  • Show Vector input if Sun Disc is unchecked

This choice is made because when the sun component is disabled there is no reason to keep the Vector socket hidden