EEVEE-Next: Resolve compilation errors in Metal #106219

Jason Fielder · 2023-03-28T15:08:55+02:00

Jason Fielder commented

2023-03-28 15:08:55 +02:00

Shader source requires explicit conversions and shader address
space qualifers in certain places in order to compile for Metal.

We also require constructors for a number of default struct types.

Authored by Apple: Michael Parkin-White

Ref #96261

Shader source requires explicit conversions and shader address space qualifers in certain places in order to compile for Metal. We also require constructors for a number of default struct types. Authored by Apple: Michael Parkin-White Ref #96261

Jason Fielder added 1 commit 2023-03-28 15:09:10 +02:00

d9a52b7bb6 EEVEE-Next: Resolve compilation errors in Metal.

Shader source requires explicit conversions and shader address
space qualifers in certain places in order to compile for Metal.

We also require constructors for a number of default struct types.

Authored by Apple: Michael Parkin-White

Ref #96261

Jason Fielder requested review from Clément Foucault 2023-03-28 15:09:37 +02:00

Jason Fielder requested review from Jeroen Bakker 2023-03-28 15:09:47 +02:00

Hans Goudey changed title from ~~EEVEE-Next: Resolve compilation errors in Metal.~~ to EEVEE-Next: Resolve compilation errors in Metal

2023-03-28 15:33:53 +02:00

Clément Foucault requested changes 2023-03-28 15:43:24 +02:00

Clément Foucault left a comment

I'm fine with most of this. Just want to see 2 points fixed before accepting this.

source/blender/draw/intern/shaders/common_aabb_lib.glsl Outdated

						
				@ -8,2 +8,4 @@

				struct AABB {

				  vec3 min, max;

				#ifdef GPU_METAL

Clément Foucault commented

2023-03-28 15:42:57 +02:00

Change caller to use the following syntax instead. Same for Sphere()

  AABB aabb;
  aabb.min = vec3(1.0e30);
  aabb.max = vec3(-1.0e30);

Change caller to use the following syntax instead. Same for `Sphere()` ``` AABB aabb; aabb.min = vec3(1.0e30); aabb.max = vec3(-1.0e30);

source/blender/draw/intern/shaders/draw_view_finalize_comp.glsl Outdated

						
				@ -74,3 +71,1 @@

				                         out vec4 top,

				                         out vec4 near,

				                         out vec4 far)

				                         DEVICE_OUT(vec4, left),

Clément Foucault commented

2023-03-28 15:40:54 +02:00

We should avoid that. Change the caller to not pass device memory references (and do some copy instead, hoping the compiler optimizes it). This make the caller code uglier but it is better than having a function that can only take device memory as argument.

Michael Parkin-White commented

2023-04-17 14:55:22 +02:00

First-time contributor

Yep, I can make this change for this case, I can't guarantee that it won't incur additional overhead, though most likely the device read needs to happen anyway, it's just a question of whether the read-write is more or less the same.

The slightly more awkward case here is that it is used in combination with the currentDEVICE_OUT_ARRAY in:
void frustum_culling_planes_calc(mat4 winmat, mat4 viewmat, DEVICE_OUT_ARRAY(vec4, planes, 6))

as this routine also then updates the planes stored in device memory.

I'm happy to refactor the code around this function to first pull the data, update locally and then update device memory if this is suitable, even if makes the code a little longer?

Yep, I can make this change for this case, I can't guarantee that it won't incur additional overhead, though most likely the device read needs to happen anyway, it's just a question of whether the read-write is more or less the same. The slightly more awkward case here is that it is used in combination with the current`DEVICE_OUT_ARRAY` in: `void frustum_culling_planes_calc(mat4 winmat, mat4 viewmat, DEVICE_OUT_ARRAY(vec4, planes, 6))` as this routine also then updates the planes stored in device memory. I'm happy to refactor the code around this function to first pull the data, update locally and then update device memory if this is suitable, even if makes the code a little longer?

Clément Foucault commented

2023-04-19 12:18:19 +02:00

The proposed way implemented in the latest version of this PR is suitable.

Jeroen Bakker requested changes 2023-04-03 08:09:19 +02:00

source/blender/draw/engines/eevee_next/shaders/eevee_light_culling_tile_comp.glsl Outdated

						
				@ -175,3 +175,4 @@

				        vec3 v001 = v000 - v_back * radius;

				        Box bbox = shape_box(v000, v100, v010, v001);

				        intersect_tile = intersect_tile && intersect(tile, bbox);

				      } break;

Jeroen Bakker commented

2023-04-03 08:07:42 +02:00

Codestyle: add break inside the code-block.

Jason Fielder added 1 commit 2023-04-17 14:41:05 +02:00

6f95280258 Merge branch 'main' into MetalEEVEENext_shaderCompilation

Michael Parkin-White added 1 commit 2023-04-17 14:55:57 +02:00

a036458c87 Merge branch 'main' into MetalEEVEENext_shaderCompilation

Jason Fielder added 1 commit 2023-04-17 16:11:30 +02:00

39ceab2071 Address feedback. Remove Metal constructors and

use struct member population for shapes.

Remove DEVICE_OUT and DEVICE_OUT_ARRAY, refactor
ViewCullingData to allow full struct copy to avoid any additional
overhead from removing device pointer modification, as device
data now copied into local variable for modification, before writing
back to device memory.

Jason Fielder added 1 commit 2023-04-17 17:51:02 +02:00

d647aa9791 Move break inside code block

Michael Parkin-White commented

2023-04-17 17:52:19 +02:00

First-time contributor

Have now addressed feedback, but given there are refactors to avoid passing device pointers directly to utility functions, and compaction of fixed size arrays into structs, the compilation needs verifying on other platforms. (This passes locally for Metal and OpenGL on macOS, though with the caveat that unsupported shaders were not tested for OpenGL, as these cannot compile on macOS anyway).

Thanks!

Have now addressed feedback, but given there are refactors to avoid passing device pointers directly to utility functions, and compaction of fixed size arrays into structs, the compilation needs verifying on other platforms. (This passes locally for Metal and OpenGL on macOS, though with the caveat that unsupported shaders were not tested for OpenGL, as these cannot compile on macOS anyway). Thanks!

Jason Fielder added 1 commit 2023-04-17 22:59:29 +02:00

6aa0011068 Host changes for ViewCullingData refactor

Michael Parkin-White added 1 commit 2023-04-17 23:00:41 +02:00

4378cdec06 Merge branch 'main' into MetalEEVEENext_shaderCompilation

Clément Foucault approved these changes 2023-04-19 15:35:07 +02:00

Clément Foucault left a comment

I tested on AMD + Linux (both Mesa & amdgpu-pro drivers) and it works.

Clément Foucault added 1 commit 2023-04-19 18:21:41 +02:00

e629d88702 Merge branch 'main' into MetalEEVEENext_shaderCompilation

Jeroen Bakker added the

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

EEVEE-Next: Resolve compilation errors in Metal #106219