Sun Disc is currently not supported because it'll need special handling - on the one hand, I'm not sure if Eevee would handle a 1e6 coming out of a background shader without issues, and on the other hand it won't actually cast sharp shadows anyways.
I guess we'd want to internally add a sun to the lamps if Sun Disc is enabled, but getting that right is tricky since the user could e.g. swap RGB channels in the node tree and the lamp wouldn't match that.
Anyways, that can be handled later, the sky itself is already a start.
Details
Details
Diff Detail
Diff Detail
- Repository
- rB Blender
- Branch
- eevee-sky (branched from master)
- Build Status
Buildable 21129 Build 21129: arc lint + arc unit
Event Timeline
Comment Actions
Having the sun disc will probably require T68478. I see that this patch touches the codegen area a bit. Since this has been refactored in eevee-rewrite branch, I would prefer this patch to be based on it. However, this would mean the inclusion would be delayed to whenever the branch gets merged to master. Maybe we want to have this feature for the next 3.1 release and just port to the branch instead.
I'm all fine with the implementation. I'm pretty sure the limit of 8 skies is plenty enough.
Comment Actions
Updated to latest master.
I've also looked into the eevee-rewrite branch, adjusting the codegen part seems quite trivial.
I can't test it since the branch doesn't appear to compile currently, but I think the best approach is to merge this into master and then merge it (with the codegen change) into eevee-rewrite.
Comment Actions
Was just reading this patch out of curiosity, but found some things to comment on.
| source/blender/gpu/intern/gpu_material.c | ||
|---|---|---|
| 113 | Looks like the patch needs clang format | |
| 113 | Looks like pixels could be a const pointer? | |
| source/blender/gpu/intern/gpu_node_graph.c | ||
| 545 | Seems a bit odd for this to free the pixels array. If it didn't, it could be a const pointer too. | |
| source/blender/nodes/shader/nodes/node_shader_tex_sky.cc | ||
| 150 | In newer code we're preferring threading::parallel_for in BLI_task.hh. Since this is already a C++ file, and the result would be simpler, maybe it's best to use that here? | |
| 248 | With my comments above, maybe it's simpler to use BLI_array.hh e.g. Array<float> pixels(4 * GPU_SKY_WIDTH * GPU_SKY_HEIGHT); | |
| 249 | designated initializers aren't part of the C++ standard until C++ 20. They don't compile on MSVC | |
Comment Actions
This is clean and simple. It can even be merge as is.
However one thing is not obvious at first glance: the content of the texture. Is it the sky radiance in a equirectangular map? or is it some preprocess LUT? I believe it doesn't include the sun disk
Also how fast is the pre-computation? is it fast enough to be interactive?
As for the sun, I believe we can still implement it later.
| source/blender/gpu/shaders/material/gpu_shader_material_tex_sky.glsl | ||
|---|---|---|
| 163–179 | This whole section could be better commented. | |
| 164–165 | style: always use curly brackets on if clause. | |
| source/blender/nodes/shader/nodes/node_shader_tex_sky.cc | ||
| 268–270 | I believe this was copied, but it could just be removed from the first assignment of eGPUSamplerState sampler. | |
Comment Actions
The content is just radiance in a equirectangular map, yes, same as in Cycles. The sun itself is not included since the resolution is not enough, in Cycles it's added in the shader stage (basically: If ray hits the sun disc, evaluate that, otherwise lookup the texture).
Regarding performance: I'm getting 11fps on my system, so interactive but not fantastic. Not sure why though, node_shader_gpu_tex_sky (incl. the entire map computation) only takes 23ms.
Comment Actions
Thanks for the update!
Tested, the sky coloring looks exactly like cycles, although I hoped that this would also bring the real light source behavior.
I get the same performance as in other sky options. To some extent it is possible to replicate behavior in cycles by adding Sun and using drivers. If that was done under the hood would be a pretty tight match.
Would this be covered in the next step (solving the sun disk issue), or is this out of the scope of this patch or a limitation of eevee?
Comment Actions
Concerning the sun disk, there is two way of looking at it:
- consider the sky model not viable without EEVEE sun extraction.
- consider the sky model not complete as it can be used with surface shader and not only world shader.
On the other hand, if the quality is not ideal, users can always disable the sun disc for EEVEE if they want to. I would say the shadowing and world lighting accuracy are a well known limitation by now so better do the right thing at the sky texture level.
| source/blender/nodes/shader/nodes/node_shader_tex_sky.cc | ||
|---|---|---|
| 39 | ICON_ERROR or ICON_INFO... | |
Comment Actions
Hi, since this patch was accepted, can we get it into master? As I understood, sky disc could be solved in subsequent patches.
In my opinion, as it is is already a huge improvement over current sitation ("yay for consistency" - in Pablo's voice )