From 7eef2a3e55481e4d62b45de6ffeee288e2f8b8f9 Mon Sep 17 00:00:00 2001 From: Michael Parkin-White Date: Wed, 12 Apr 2023 12:16:27 +0100 Subject: [PATCH] Fix #106773: resolve Metal grease pencil fill Changes to viewport state to resolve texture paint color selection introduced a side effect wherein the correct attachment size of a framebuffer was reset. This size is needed when scissor regions are disabled to return the state to its correct default. When this default was wrong, certain operators would have incorrect offsets. To maintain consistency with the OpenGL backend, the Metal backend independently tracks the raw attachment size using default_width/height. This will also reset to zero when attachments are all removed, unlike other state which may be retained. Authored by Apple: Michael Parkin-White --- source/blender/gpu/metal/mtl_context.mm | 19 +++-- source/blender/gpu/metal/mtl_framebuffer.hh | 20 +++++ source/blender/gpu/metal/mtl_framebuffer.mm | 93 +++++++++++++-------- 3 files changed, 87 insertions(+), 45 deletions(-) diff --git a/source/blender/gpu/metal/mtl_context.mm b/source/blender/gpu/metal/mtl_context.mm index 57ca18c4edc..a89b1562396 100644 --- a/source/blender/gpu/metal/mtl_context.mm +++ b/source/blender/gpu/metal/mtl_context.mm @@ -949,20 +949,23 @@ bool MTLContext::ensure_render_pipeline_state(MTLPrimitiveType mtl_prim_type) /* Some scissor assignments exceed the bounds of the viewport due to implicitly added * padding to the width/height - Clamp width/height. */ - BLI_assert(scissor.x >= 0 && scissor.x < render_fb->get_width()); - BLI_assert(scissor.y >= 0 && scissor.y < render_fb->get_height()); - scissor.width = min_ii(scissor.width, render_fb->get_width() - scissor.x); - scissor.height = min_ii(scissor.height, render_fb->get_height() - scissor.y); - BLI_assert(scissor.width > 0 && (scissor.x + scissor.width <= render_fb->get_width())); - BLI_assert(scissor.height > 0 && (scissor.height <= render_fb->get_height())); + BLI_assert(scissor.x >= 0 && scissor.x < render_fb->get_default_width()); + BLI_assert(scissor.y >= 0 && scissor.y < render_fb->get_default_height()); + scissor.width = (uint)min_ii(scissor.width, + max_ii(render_fb->get_default_width() - (int)(scissor.x), 0)); + scissor.height = (uint)min_ii( + scissor.height, max_ii(render_fb->get_default_height() - (int)(scissor.y), 0)); + BLI_assert(scissor.width > 0 && + (scissor.x + scissor.width <= render_fb->get_default_width())); + BLI_assert(scissor.height > 0 && (scissor.height <= render_fb->get_default_height())); } else { /* Scissor is disabled, reset to default size as scissor state may have been previously * assigned on this encoder. */ scissor.x = 0; scissor.y = 0; - scissor.width = render_fb->get_width(); - scissor.height = render_fb->get_height(); + scissor.width = render_fb->get_default_width(); + scissor.height = render_fb->get_default_height(); } /* Scissor state can still be flagged as changed if it is toggled on and off, without diff --git a/source/blender/gpu/metal/mtl_framebuffer.hh b/source/blender/gpu/metal/mtl_framebuffer.hh index 2d3d5fb0730..8751fa956e9 100644 --- a/source/blender/gpu/metal/mtl_framebuffer.hh +++ b/source/blender/gpu/metal/mtl_framebuffer.hh @@ -104,6 +104,16 @@ class MTLFrameBuffer : public FrameBuffer { /** Whether the primary Frame-buffer attachment is an SRGB target or not. */ bool srgb_; + /** Default width/height represent raw size of active framebuffer attachments. + * For consistency with OpenGL backend, as width_/height_ can affect viewport and scissor + * size, we need to track this differently to ensure viewport state does not get reset. + * This size is only used to reset viewport/scissor regions when viewports and scissor are + * disabled, as Metal does not provide a utility to fully disable either without manually + * specifying the size. + */ + int default_width_ = 0; + int default_height_ = 0; + public: /** * Create a conventional framebuffer to attach texture to. @@ -157,6 +167,7 @@ class MTLFrameBuffer : public FrameBuffer { /* Attachment management. */ /* When dirty_attachments_ is true, we need to reprocess attachments to extract Metal * information. */ + void ensure_attachments_and_viewport(); void update_attachments(bool update_viewport); bool add_color_attachment(gpu::MTLTexture *texture, uint slot, int miplevel, int layer); bool add_depth_attachment(gpu::MTLTexture *texture, int miplevel, int layer); @@ -211,6 +222,9 @@ class MTLFrameBuffer : public FrameBuffer { int get_width(); int get_height(); + int get_default_width(); + int get_default_height(); + bool get_dirty() { return is_dirty_ || is_loadstore_dirty_; @@ -231,6 +245,12 @@ class MTLFrameBuffer : public FrameBuffer { return srgb_; } + inline void default_size_set(int w, int h) + { + default_width_ = w; + default_height_ = h; + } + private: /* Clears a render target by force-opening a render pass. */ void force_clear(); diff --git a/source/blender/gpu/metal/mtl_framebuffer.mm b/source/blender/gpu/metal/mtl_framebuffer.mm index b2aea30d57d..e2ef7cc027e 100644 --- a/source/blender/gpu/metal/mtl_framebuffer.mm +++ b/source/blender/gpu/metal/mtl_framebuffer.mm @@ -90,6 +90,18 @@ MTLFrameBuffer::~MTLFrameBuffer() } } +void MTLFrameBuffer::ensure_attachments_and_viewport() +{ + /* Ensure local MTLAttachment data is up to date. + * NOTE: We only refresh viewport/scissor region when attachments are updated during bind. + * This is to ensure state is consistent with the OpenGL backend. */ + if (dirty_attachments_) { + this->update_attachments(true); + this->viewport_reset(); + this->scissor_reset(); + } +} + void MTLFrameBuffer::bind(bool enabled_srgb) { @@ -102,11 +114,7 @@ void MTLFrameBuffer::bind(bool enabled_srgb) /* Ensure local MTLAttachment data is up to date. * NOTE: We only refresh viewport/scissor region when attachments are updated during bind. * This is to ensure state is consistent with the OpenGL backend. */ - if (dirty_attachments_) { - this->update_attachments(true); - this->viewport_reset(); - this->scissor_reset(); - } + this->ensure_attachments_and_viewport(); /* Ensure SRGB state is up-to-date and valid. */ bool srgb_state_changed = enabled_srgb_ != enabled_srgb; @@ -134,8 +142,10 @@ void MTLFrameBuffer::bind(bool enabled_srgb) bool MTLFrameBuffer::check(char err_out[256]) { - /* Ensure local MTLAttachment data is up to date. */ - this->update_attachments(true); + /* Ensure local MTLAttachment data is up to date. + * NOTE: We only refresh viewport/scissor region when attachments are updated during bind. + * This is to ensure state is consistent with the OpenGL backend. */ + this->ensure_attachments_and_viewport(); /* Ensure there is at least one attachment. */ bool valid = (this->get_attachment_count() > 0 || @@ -543,8 +553,8 @@ void MTLFrameBuffer::blit_to(eGPUFrameBufferBits planes, int dst_offset_x, int dst_offset_y) { - this->update_attachments(true); - static_cast(dst)->update_attachments(true); + this->ensure_attachments_and_viewport(); + static_cast(dst)->ensure_attachments_and_viewport(); BLI_assert(planes != 0); @@ -773,8 +783,8 @@ void MTLFrameBuffer::apply_state() if (viewport_w == 0 || viewport_h == 0) { MTL_LOG_WARNING( "Viewport had width and height of (0,0) -- Updating -- DEBUG Safety check\n"); - viewport_w = width_; - viewport_h = height_; + viewport_w = default_width_; + viewport_h = default_height_; } /* Update Context State. */ @@ -892,7 +902,7 @@ bool MTLFrameBuffer::add_color_attachment(gpu::MTLTexture *texture, break; } - /* Update Frame-buffer Resolution. */ + /* Update default attachment size and ensure future attachments match the same size. */ int width_of_miplayer, height_of_miplayer; if (miplevel <= 0) { width_of_miplayer = texture->width_get(); @@ -903,14 +913,14 @@ bool MTLFrameBuffer::add_color_attachment(gpu::MTLTexture *texture, height_of_miplayer = max_ii(texture->height_get() >> miplevel, 1); } - if (width_ == 0 || height_ == 0) { - this->size_set(width_of_miplayer, height_of_miplayer); - BLI_assert(width_ > 0); - BLI_assert(height_ > 0); + if (default_width_ == 0 || default_height_ == 0) { + this->default_size_set(width_of_miplayer, height_of_miplayer); + BLI_assert(default_width_ > 0); + BLI_assert(default_height_ > 0); } else { - BLI_assert(width_ == width_of_miplayer); - BLI_assert(height_ == height_of_miplayer); + BLI_assert(default_width_ == width_of_miplayer); + BLI_assert(default_height_ == height_of_miplayer); } /* Flag as dirty. */ @@ -1011,7 +1021,7 @@ bool MTLFrameBuffer::add_depth_attachment(gpu::MTLTexture *texture, int miplevel break; } - /* Update Frame-buffer Resolution. */ + /* Update default attachment size and ensure future attachments match the same size. */ int width_of_miplayer, height_of_miplayer; if (miplevel <= 0) { width_of_miplayer = texture->width_get(); @@ -1022,15 +1032,14 @@ bool MTLFrameBuffer::add_depth_attachment(gpu::MTLTexture *texture, int miplevel height_of_miplayer = max_ii(texture->height_get() >> miplevel, 1); } - /* Update Frame-buffer Resolution. */ - if (width_ == 0 || height_ == 0) { - this->size_set(width_of_miplayer, height_of_miplayer); - BLI_assert(width_ > 0); - BLI_assert(height_ > 0); + if (default_width_ == 0 || default_height_ == 0) { + this->default_size_set(width_of_miplayer, height_of_miplayer); + BLI_assert(default_width_ > 0); + BLI_assert(default_height_ > 0); } else { - BLI_assert(width_ == texture->width_get()); - BLI_assert(height_ == texture->height_get()); + BLI_assert(default_width_ == width_of_miplayer); + BLI_assert(default_height_ == height_of_miplayer); } /* Flag as dirty after attachments changed. */ @@ -1131,7 +1140,7 @@ bool MTLFrameBuffer::add_stencil_attachment(gpu::MTLTexture *texture, int miplev break; } - /* Update Frame-buffer Resolution. */ + /* Update default attachment size and ensure future attachments match the same size. */ int width_of_miplayer, height_of_miplayer; if (miplevel <= 0) { width_of_miplayer = texture->width_get(); @@ -1142,15 +1151,14 @@ bool MTLFrameBuffer::add_stencil_attachment(gpu::MTLTexture *texture, int miplev height_of_miplayer = max_ii(texture->height_get() >> miplevel, 1); } - /* Update Frame-buffer Resolution. */ - if (width_ == 0 || height_ == 0) { - this->size_set(width_of_miplayer, height_of_miplayer); - BLI_assert(width_ > 0); - BLI_assert(height_ > 0); + if (default_width_ == 0 || default_height_ == 0) { + this->default_size_set(width_of_miplayer, height_of_miplayer); + BLI_assert(default_width_ > 0); + BLI_assert(default_height_ > 0); } else { - BLI_assert(width_ == texture->width_get()); - BLI_assert(height_ == texture->height_get()); + BLI_assert(default_width_ == width_of_miplayer); + BLI_assert(default_height_ == height_of_miplayer); } /* Flag as dirty after attachments changed. */ @@ -1229,8 +1237,8 @@ void MTLFrameBuffer::ensure_render_target_size() if (colour_attachment_count_ == 0 && !this->has_depth_attachment() && !this->has_stencil_attachment()) { - /* Reset size for empty framebuffer. */ - this->size_set(0, 0); + /* Reset default size for empty framebuffer. */ + this->default_size_set(0, 0); } } @@ -1461,7 +1469,9 @@ bool MTLFrameBuffer::validate_render_pass() BLI_assert(this); /* First update attachments if dirty. */ - this->update_attachments(true); + if (dirty_attachments_) { + this->update_attachments(true); + } /* Verify attachment count. */ int used_attachments = 0; @@ -1892,4 +1902,13 @@ int MTLFrameBuffer::get_height() return height_; } +int MTLFrameBuffer::get_default_width() +{ + return default_width_; +} +int MTLFrameBuffer::get_default_height() +{ + return default_height_; +} + } // namespace blender::gpu -- 2.30.2