Page MenuHome

Sky Texture fixes
ClosedPublic

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

Details

Summary

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

  • Add Sun Intensity parameter:

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

  • Remove limit for Sun Rotation parameter:

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

  • Remove Vector input when sun disc is enabled:

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;

  • Change Altitude parameter from meters to kilometers:

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

  • 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);

  • Fix sun issue at Altitude 0:

the problem was given because the rays shot by the virtual camera that samples the precomputed texture could intersect with the Earth surface giving artifacts such as https://developer.blender.org/T78032. Clamping the altitude internally 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, simple versioning fixed it.

Diff Detail

Repository
rB Blender

Event Timeline

Marco (nacioss) requested review of this revision.Jun 22 2020, 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.Jun 22 2020, 2:45 PM
Marco (nacioss) edited the summary of this revision. (Show Details)
Marco (nacioss) edited the summary of this revision. (Show Details)Jun 22 2020, 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)Jun 26 2020, 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)Jun 27 2020, 4:58 PM
Ankit Meel (ankitm) retitled this revision from Sky Texture fixes to [WIP] Sky Texture fixes.Jun 27 2020, 6:00 PM
Lukas Stockner (lukasstockner97) requested changes to this revision.Jul 2 2020, 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 ↗(On Diff #26322)

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

intern/cycles/kernel/svm/svm_sky.h
183 ↗(On Diff #26322)

Same here (safe_sqrtf outside of OSL).

intern/cycles/render/nodes.cpp
764–769

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

intern/cycles/util/util_sky_nishita.cpp
298 ↗(On Diff #26322)

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

This revision now requires changes to proceed.Jul 2 2020, 12:58 AM
  • Cleanup: Lukas changes
Marco (nacioss) marked 4 inline comments as done.Jul 2 2020, 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

Just pushing my two cents here:

  • The sun sampling code now takes the Texture Mapping settings of the Sky Texture into account.
  • Versioning was fixed to convert the altitude and to initialize sun intensity to 1 on older files. I also moved it from versioning_cycles.c to versioning_290.c since versioning_cycles.c appears to be for Cycles-defined properties (e.g. properties.py), while the texture settings are normal DNA data.
  • Rebased to include the T78324 fix.

As usual, someone else will need to review my changes.

Also, I'm not sure what splitting the changes has to do with commit rights...

This looks fine except for one comment which I think @Lukas Stockner (lukasstockner97) can address along with committing.

intern/cycles/blender/blender_shader.cpp
822 ↗(On Diff #26613)

Do this clamping in nodes.cpp, so all applications using Cycles get this test.

Marco (nacioss) retitled this revision from [WIP] Sky Texture fixes to Sky Texture fixes.Jul 8 2020, 12:50 PM
Marco (nacioss) edited the summary of this revision. (Show Details)
Marco (nacioss) updated this revision to Diff 26670.EditedJul 9 2020, 5:46 PM