This is a port of the OSL Sky Texture to the OpenGL viewport.
Attached below is a test scene. Just switch one of the 3D viewports to rendered mode and play with the texture values/camera.
Details
Details
- Reviewers
Sergey Sharybin (sergey)
Diff Detail
Diff Detail
- Repository
- rB Blender
Event Timeline
Comment Actions
It works great :) A pleasure to see Cycles viewport step by step closer to BI's viewport.
Comment Actions
Didn't apply or test the patch, but having copy of util_sky_model.{h,c} seems weak to me. First of all, it's not code we maintain, so technically it should be moved to extern/. Ideally we also need to de-duplicate it with Cycles, but that would make it more tricky to have standalone Cycles (tho, we've got third_party sources directory there already, so could be all fine but is out of the scope of this particular patch i think).
That's current feedback against current master,
Now, more tricky speculation. We are working on full viewport and render/shader integration, so not sure if this fits really well into our plans to move forward with 2.8 branch. This is where having some feedback from @Mike Erwin (merwin) will be really good ;)
| source/blender/gpu/shaders/gpu_shader_material.glsl | ||
|---|---|---|
| 3409 | Please don't combine condition with action in the same line. Also, we are tempting to always use parenthesis to reduce chances of errors and diff noise in the future. | |
| 3458 | What about using proper sentence for comments (start with capital and end with full-stop)? | |
Comment Actions
I also have not tested this patch yet. Agree that we should have only 1 copy of this code, will leave it to @Sergey Sharybin (sergey) to decide where that single copy should live.
Now, more tricky speculation. We are working on full viewport and render/shader integration, so not sure if this fits really well into our plans to move forward with 2.8 branch. This is where having some feedback from @Mike Erwin (merwin) will be really good ;)
My first reaction is to put this in master for 2.79, then we can make necessary changes in the 2.8 branch to fit it into the (in flux) design. Shader gen for materials will be reworked for 2.8, but the GLSL itself will stay the same.
Also to clarify about licensing and authorship: It looks like util_sky_model C files are from the researchers, GLSL and node setup is original code from @Ralf Hölzemer (cheleb). Is that correct?
| source/blender/gpu/shaders/gpu_shader_material.glsl | ||
|---|---|---|
| 3409 | Since it's assigning a single variable in both cases, you could write float X = (y == 0.0) ? 0.0 : (x / y * Y); float Z = (y == 0.0 || Y == 0.0) ? 0.0 : ((1.0 - x - y) / y * Y); return vec3(X, Y, Z); | |
| 3458 | I disagree, preferring comments to say just what needs saying. Sometimes that's a sentence but other times a phrase or fragment is ok. | |
| source/blender/nodes/shader/nodes/node_shader_tex_sky.c | ||
| 141 | Using clamp function here would make it more readable. We have CLAMP, CLAMP_MIN, CLAMP_MAX available. | |
Comment Actions
I am against putting this into e.g. extern/. It's part of Cycles SVM/OSL shading system and we should not start outsourcing such things here imho. Then I'd rather have these files twice in the source tree (once Cycles, once GLSL).
Comment Actions
I'd also be inclined to just keep the file where it is, not sure putting it in extern/ actually makes things easier for anyone.
Hopefully we can remove the Preetham model in Blender 2.8 as well, to simplify this code a bit.
Comment Actions
I also value simplicity. From a dev perspective it would be nice if the old model can be dropped and we have one "official" sky model. Do we know if the Preetham model is in wide use?
On the other point: Having two different sky models seems ok though not ideal to me, but having two copies of the exact same code in Blender... seems like a problem we can (and should) solve.
Cycles uses sky_model
Blender uses Cycles
Blender also uses sky_model (with this patch)
I'm ok with the sky_model code remaining in Cycles but accessible (via C) to the viewport shader gen function that also uses it. For builds with Cycles disabled, we can also disable shader gen for this node. Did I miss any issues here?
Comment Actions
There is already a patch for Eevee to port the Sky models from Cycles here https://developer.blender.org/D7108
I think this patch review can be closed.