Details

Reviewers

Clément Foucault (fclem)
Brecht Van Lommel (brecht)

Commits

rB9de09220fc5f: EEVEE: Implement the missing Sky texture

Summary

I'm not sure if the Sky was deliberately left out or was just waiting for a
better moment, but so many I was disappointed that Sky in EEVEE is
completely white.

There are already 2 implementations (osl and gpu) so this is the third one.
Looking at other cases it seems that we are not supposed to share sources
between cycles and the rest? So the new util_sky_model files are just
copies of what is already in cycles, except that the data file uses the RGB
variant of the Hosek/Wilkie model, because we output RGB anyway (but can be
easily changed to XYZ if desired - the results are nearly identical).
I am not sure if it is okay to pass 3*9 float values as 3 mat4 uniforms (I
wanted to use mat3 but it does not work).
Also, should I cache the sky model data between renders if the parameters
do not change?

Diff Detail

Repository

rB Blender

Branch

eevee-sky (branched from master)

Build Status

Buildable 8842
Build 8842: arc lint + arc unit

Event Timeline

Szymon Ulatowski (szulat) created this revision.Mar 11 2020, 1:30 PM

Harbormaster completed remote builds in B7089: Diff 22673.Mar 11 2020, 1:30 PM

Szymon Ulatowski (szulat) retitled this revision from Implement the missing Sky texture in EEVEE to EEVEE: Implement the missing Sky texture.Mar 11 2020, 1:49 PM

Szymon Ulatowski (szulat) edited the summary of this revision. (Show Details)

Szymon Ulatowski (szulat) added a reviewer: Clément Foucault (fclem).

Szymon Ulatowski (szulat) added a project: Eevee.

higgsas (higgsas) awarded a token.Mar 11 2020, 1:50 PM

higgsas (higgsas) added a subscriber: higgsas (higgsas).

I did not test the functionality yet. This is a quick style review.

@Brecht Van Lommel (brecht) are the util files at the right place?

source/blender/gpu/shaders/material/gpu_shader_material_tex_sky.glsl
5	always use parenthesis on ifs
33	parenthesis
48	use consts here
source/blender/nodes/shader/nodes/node_shader_tex_sky.c
64	use cosf
67	use expf
141	remove empty lines
158	for better ubo packing, please use vec4 + float
178	same here

Clément Foucault (fclem) added inline comments.Mar 11 2020, 4:11 PM

source/blender/nodes/shader/nodes/node_shader_tex_sky.c
148–214	you could use 6vec4 + 1vec3 to pass all that data. I know it's not ideal but using mat4 is wasting too much data.

Clément Foucault (fclem) awarded a token.Mar 11 2020, 4:15 PM

noki paike (amonpaike) awarded a token.Mar 11 2020, 8:37 PM

Using more vec4, using cosf()/expf(), formatting.

Harbormaster completed remote builds in B7108: Diff 22708.Mar 11 2020, 11:50 PM

Szymon Ulatowski (szulat) marked 9 inline comments as done.Mar 11 2020, 11:53 PM

Looks really good! I wanted to tackle this for a long time already but always had something else to do. Thanks for the patch!

I'm just unsure about the duplication of the sky model files.

Also, should I cache the sky model data between renders if the parameters do not change?

This does not seems to be an issue.

This revision is now accepted and ready to land.Mar 12 2020, 5:37 PM

Jason Clarke (jtheninja) added a subscriber: Jason Clarke (jtheninja).Mar 12 2020, 6:33 PM

Bartosz Moniewski (monio) awarded a token.Mar 12 2020, 10:20 PM

Bartosz Moniewski (monio) added a subscriber: Bartosz Moniewski (monio).

Wo!262 (wo262) awarded a token.Mar 12 2020, 11:56 PM

Fran Marz (franMarz) awarded a token.Mar 15 2020, 8:07 AM

In D7108#170147, @Clément Foucault (fclem) wrote:

I'm just unsure about the duplication of the sky model files.

I think it's ok. We could try to deduplicate it but I'm not sure that really makes anything simpler, especially for Cycles standalone.

source/blender/gpu/shaders/material/gpu_shader_material_tex_sky.glsl
54–56	Don't hardcode this, use `imbuf_xyz_to_rgb` matrix so it works with different color spaces.

Szymon Ulatowski (szulat) added inline comments.Apr 10 2020, 3:34 AM

source/blender/gpu/shaders/material/gpu_shader_material_tex_sky.glsl
54–56	Can other eevee materials output colors other than rgb? For me it seemed like everyone outputs rgb and only then it is sometimes converted to something else (but don't hold my word for it). Anyway, now that I found gpu_shader_material_color_util.glsl (containing hsv_to_rgb among others), it appears to be a much better place for xyz_to_rgb() than the sky shader glsl.

Brecht Van Lommel (brecht) added inline comments.Apr 10 2020, 4:19 AM

source/blender/gpu/shaders/material/gpu_shader_material_tex_sky.glsl
54–56	RGB is not a single color space. Different RGB color spaces can be used through OpenColorIO configurations, for example ACEScg.

Allie (ChildishGiant) awarded a token.Apr 27 2020, 11:23 PM

Allie (ChildishGiant) added a subscriber: Allie (ChildishGiant).Apr 27 2020, 11:34 PM

Marco (nacioss) awarded a token.Jun 21 2020, 3:07 PM

Clément Foucault (fclem) added inline comments.Jun 29 2020, 10:17 PM

source/blender/gpu/shaders/material/gpu_shader_material_tex_sky.glsl
54–56	You can pass the imbuf_xyz_to_rgb matrix as 3 *vec3 uniforms to the shader.

use 'imbuf_xyz_to_rgb' as uniform instead of literal constants

I haven't found a way to make blender use anything other than sRGB linear as
its internal color format in eevee nodes but if it can really change then
sure, let's use IMB_colormanagement. This also means that for correctness, I
should also change the other sky model to calculate in the XYZ color space
and convert to rgb using the same matrix (currently the direct sRGB version
is used (unlike in cycles) because I thought that avoiding the additional
conversion would be an advantage).
And the next step is of course porting the new Nishita model :)

(in the meantime I was checking if there is a way of to add uniforms in GLSL
library functions (so for example, xyz_to_rgb() would only consume the
uniform matrix once, even when used by 5 instances of the sky texture) and
it seems than the set of builtins (gpu_codegen.c) is not easily
extendable(?))

Harbormaster completed remote builds in B8835: Diff 26519.Jul 4 2020, 3:12 AM

Szymon Ulatowski (szulat) marked an inline comment as done.Jul 4 2020, 3:14 AM

(in the meantime I was checking if there is a way of to add uniforms in GLSL library functions (so for example, xyz_to_rgb() would only consume the uniform matrix once, even when used by 5 instances of the sky texture) and it seems than the set of builtins (gpu_codegen.c) is not easily extendable(?))

Yes it's a known issue but that should not prevent this patch from being merged.

I propose to port the new model in a separate diff.

All seems right to me. @Brecht Van Lommel (brecht) any remaining complain?

I haven't found a way to make blender use anything other than sRGB linear as its internal color format in eevee nodes but if it can really change then sure, let's use IMB_colormanagement.

It can't be changed from within the Blender UI, but if the OpenColorIO config defines the "XYZ" role it will be read from that. As Brecht noted, getting the conversion from the color management system is required for the shader to behave properly under ACES

If you want an OpenColorIO config with a different rendering space (and the XYZ role setup) for testing you can find one here: https://github.com/sobotka/ACES-1.2-Displays-Formatted/tree/blender-configuration

Alaska (Alaska) awarded a token.Jul 4 2020, 8:55 AM

If you want an OpenColorIO config with a different rendering space (and the XYZ role setup) for testing you can find one here: https://github.com/sobotka/ACES-1.2-Displays-Formatted/tree/blender-configuration

thank you, that's exactly what I needed for the proper testing!

Use the XYZ colorspace variant of the Hosek-Wilkie sky

Harbormaster completed remote builds in B8842: Diff 26526.Jul 4 2020, 5:24 PM

Hey, congrats for the work you did here, i'm the Nishita Sky guy and would like to talk with you about porting the model in Eevee, do you have an email or something so i can contact you? Otherwise here's my email wolf.art3d@gmail.com
Anyway as said by Clement, the porting of Nishita should be a different patch.

By the way, just tested the patch and it works fine!

In D7108#199586, @Marco (nacioss) wrote:

Hey, congrats for the work you did here

Thanks :-) just a translation from cycles to eeve, nothing fancy! I don't even insist I should be the one to port Nishita sky to eevee but of course I'd like to try in case noone else is already working on it. My email is szulat@gmail.com

Marco (nacioss) mentioned this in D2132: Enable Cycles Sky Texture in OpenGL viewport.Jul 7 2020, 3:18 PM

Reunify 'util_sky_model' for cycles and eevee (reduce code duplication)

This work was done mostly by @Marco (nacioss) with a little help from me :)
CMakeLists.txt should be probably added inside intern/sky but we couldn't
figure out how.
minimal_float3.h is mostly a small portion of blenlib/BLI_float3.hh so the
Nishita sky model can compile without any dependencies. Can we do better?

Harbormaster completed remote builds in B8893: Diff 26608.Jul 7 2020, 10:58 PM

Andrei Salamandic (andreymd87) awarded a token.Jul 8 2020, 9:17 AM

This needs a CMakeLists.txt to create the corresponding library. intern/numaapi is a good example since it's also used by both Blender and Cycles. bf_intern_sky will then needed to be added in intern/cycles/app/CMakeLists.txt and source/blender/nodes/CMakeLists.txt as dependencies.
Files should be renamed to just sky_*., since util_ makes little sense outside of that Cycles folder.
The intern/sky folder needs to have separate include and source folders, for the public header files and internal implementation respectively.
minimal_float3.h should also get a sky_ prefix, and only get used internally and not be part of the public API.
Public functions names and types can also be changed to all get a SKY_* prefix.

And there's probably some more things I forgot, basically make it work like other modules in intern.

This revision now requires changes to proceed.Jul 8 2020, 11:27 AM

build intern/sky as a library, rename and move sources as required by the module standards
add SKY_ prefix in public function names and types
get rid of the warnings in IMB_colormanagement_get_xyz_to_rgb()

Like yesterday, the patch was made in cooperation with @Marco (nacioss) !

Harbormaster completed remote builds in B8919: Diff 26644.Jul 8 2020, 8:06 PM

The Cycles side of this looks basically fine, mainly up to @Clément Foucault (fclem) now to check the Eevee side.

intern/cycles/render/image_sky.cpp
19 ↗	(On Diff #26644)	Add empty line after this.
intern/cycles/render/nodes.cpp
30 ↗	(On Diff #26644)	Use `#include "sky_model.h"`. Also, add an empty line after this so includes are grouped by module.
intern/sky/source/sky_float3.h
17–18 ↗	(On Diff #26644)	MINIMAL -> SKY