Compilation errors are logged to console ?

If the shader was bound using GPU_shader_bind one would intuitively expect GPU_shader_unbind be used to unbind it.

I do not see the point of such shortening.

Use min and max from the Cycles math.h utilities.

This is not really an ID anymore.

Don't just shorten existing variables.
In general, don't value saving horizontal space over ease and clarity of reading by a new developer.

Why the 256 bytes is so important? What does it mean "to enable allocation" ?

While currently this is OK, more correct would be be to use size_t instead of uint here.

Use max from the ccl namespace.
Within the current state of the code it will avoid implicit sign cast. Once the required_size becomes size_t doing so will avoid implicit narrowing.

Is this something planned for this patch?

Would it make sense, or even possible, to allocate fences as part of the rest of the GPU resources (as opposite of doing on-demand) ?

Why was this changed to the immediate mode emulation?

GPU module shader compilation should have its own error logger via gl_shader_log.cc for OpenGL. Would this suffice, or is other logging preferred?

Thanks!

For Metal, minimum buffer allocation size generally needs to be 256 bytes. This detail could be moved to the backend, but the key is to ensure that we can create a valid allocation, if the is what the remaining code expects.

This is also a catch-all against a zero-sized guard, to ensure that the path functions as expected. The alternative here would be to guard creation if either dimension ends up as zero.

Let me know if there is a preference either way?

Will update, thanks

Ah yes, this is possibly left open to discussion on whether GPUVertBuf via GPUBatch is preferred over GPU_Immediate?

Given this is effectively just drawing a quad, the performance overhead of GPU_Immediate is negligible and this also simplifies the implementation detail. However, using GPUVertBuf may be preferred to keep the sentiment of the original code.

Please let me know what the preference is?

Ah yes, this is probably a safe bet. However the pattern does seem to vary a little.
In source/blender/draw/intern/draw_manager_shader.c, WM_opengl_context_activate is called after context creation.

comp->gl_context = WM_opengl_context_create();
comp->gpu_context = GPU_context_create(NULL);
GPU_context_active_set(NULL);

WM_opengl_context_activate(DST.gl_context);
GPU_context_active_set(DST.gpu_context);

Though ensuring this context is active prior to GPU Context creation appears to make more sense, to ensure the backing OpenGL context is active before resources are created. Should be reasonable to modify the code accordingly.

This was done for simplicity of the code, as it would seem the cost overhead of performing GPUBatch rendering with a pre-baked vertex buffer is more expensive than using GPUImmediate for very small vertex counts, especially if they are subject to data changing relatively frequently.

However, please let me know if you prefer this to be written using GPUBatch + GPUVertBuf, as this can be changed.

From testing, adding a WM_opengl_context_activate(comp->gl_context) before GPU_context_creation fails on the assertion:

BLI_assert(GPU_framebuffer_active_get() == GPU_framebuffer_back_get());``` due to having no valid GPU context active at the time.

```void WM_opengl_context_activate(void *context)
{
  BLI_assert(GPU_framebuffer_active_get() == GPU_framebuffer_back_get());
  GHOST_ActivateOpenGLContext((GHOST_ContextHandle)context);
}

As DRW_opengl_context_release() called above disables the active GPU_context, which is later restored in the call to: DRW_opengl_context_activate

Given the code does not immediately require usage of the newly created GPU_context, then perhaps this is not a problem as-is. However, if we did want to ensure the backing GL context was active during context creation, then we could:

1. Skip the default framebuffer assertion in WM_opengl_context_activate if there is no valid GPUContext active.

2. Call GHOST_ActivateOpenGLContext directly, though this perhaps feels a bit messy as it opens the door for multiple ways of doing the same thing.

Or, please let me know if there is an alternative preferred approach for this.

Thanks!

Can become simply GPU_fence_wait(gpu_render_sync_); (without check) ?

Use DCHECK_NOTNULL/CHECK_NOTNULL check that something is not null.

Thanks for the explanation. If there is no speed penalty on both OpenGL and Metal, and the GPU_immediate.h is not planned to be deprecated with the coming development in the GPU module your suggested code is indeed simpler.

Add an assert, or check for nullptr and return false?

Also, if the sync objects are created here, checks before sync calls can be removed in the file?