Vulkan is strict about scissor rect, so let's clamp centrally.

Better than spreading the logic all over. Fixes #15207
hrydgard · Dec 8, 2021 · 45308a1 · 45308a1
1 parent dbfa4e6
commit 45308a1
Show file tree

Hide file tree

Showing 8 changed files with 49 additions and 40 deletions.
diff --git a/Common/GPU/Vulkan/VulkanQueueRunner.cpp b/Common/GPU/Vulkan/VulkanQueueRunner.cpp
@@ -1337,8 +1337,6 @@ void VulkanQueueRunner::PerformRenderPass(const VKRStep &step, VkCommandBuffer c
 			} else {
 				// Rendering to backbuffer. Might need to rotate.
 				const VkRect2D &rc = c.scissor.scissor;
-				_dbg_assert_(rc.offset.x >= 0);
-				_dbg_assert_(rc.offset.y >= 0);
 				DisplayRect<int> rotated_rc{ rc.offset.x, rc.offset.y, (int)rc.extent.width, (int)rc.extent.height };
 				RotateRectToDisplay(rotated_rc, vulkan_->GetBackbufferWidth(), vulkan_->GetBackbufferHeight());
 				_dbg_assert_(rotated_rc.x >= 0);

diff --git a/Common/GPU/Vulkan/VulkanRenderManager.h b/Common/GPU/Vulkan/VulkanRenderManager.h
@@ -289,33 +289,41 @@ class VulkanRenderManager {
 		curStepHasViewport_ = true;
 	}
 
-	void SetScissor(VkRect2D rc) {
+	// It's OK to set scissor outside the valid range - the function will automatically clip.
+	void SetScissor(int x, int y, int width, int height) {
 		_dbg_assert_(curRenderStep_ && curRenderStep_->stepType == VKRStepType::RENDER);
-		_dbg_assert_((int)rc.offset.x >= 0);
-		_dbg_assert_((int)rc.offset.y >= 0);
-		_dbg_assert_((int)rc.extent.width >= 0);
-		_dbg_assert_((int)rc.extent.height >= 0);
-
-		// Clamp to curWidth_/curHeight_. Apparently an issue.
-		if ((int)(rc.offset.x + rc.extent.width) > curWidth_) {
-			int newWidth = curWidth_ - rc.offset.x;
-			rc.extent.width = std::max(1, newWidth);
-			if (rc.offset.x >= curWidth_) {
-				// Fallback.
-				rc.offset.x = std::max(0, curWidth_ - (int)rc.extent.width);
-			}
+
+		if (x < 0) {
+			width += x;  // since x is negative, this shrinks width.
+			x = 0;
+		}
+		if (y < 0) {
+			height += y;
+			y = 0;
+		}
+
+		if (x + width > curWidth_) {
+			width = curWidth_ - x;
+		}
+		if (y + height > curHeight_) {
+			height = curHeight_ - y;
 		}
 
-		if ((int)(rc.offset.y + rc.extent.height) > curHeight_) {
-			int newHeight = curHeight_ - rc.offset.y;
-			rc.extent.height = std::max(1, newHeight);
-			if (rc.offset.y >= curHeight_) {
-				// Fallback.
-				rc.offset.y = std::max(0, curHeight_ - (int)rc.extent.height);
-			}
+		// Check validity.
+		if (width < 0 || height < 0 || x >= curWidth_ || y >= curHeight_) {
+			// TODO: If any of the dimensions are now zero or negative, we should flip a flag and not do draws, probably.
+			// Instead, if we detect an invalid scissor rectangle, we just put a 1x1 rectangle in the upper left corner.
+			x = 0;
+			y = 0;
+			width = 1;
+			height = 1;
 		}
 
-		// TODO: If any of the dimensions are now zero, we should flip a flag and not do draws, probably.
+		VkRect2D rc;
+		rc.offset.x = x;
+		rc.offset.y = y;
+		rc.extent.width = width;
+		rc.extent.height = height;
 
 		curRenderArea_.Apply(rc);
 

diff --git a/Common/GPU/Vulkan/thin3d_vulkan.cpp b/Common/GPU/Vulkan/thin3d_vulkan.cpp
@@ -1153,8 +1153,7 @@ Pipeline *VKContext::CreateGraphicsPipeline(const PipelineDesc &desc) {
 }
 
 void VKContext::SetScissorRect(int left, int top, int width, int height) {
-	VkRect2D scissor{ {(int32_t)left, (int32_t)top}, {(uint32_t)width, (uint32_t)height} };
-	renderManager_.SetScissor(scissor);
+	renderManager_.SetScissor(left, top, width, height);
 }
 
 void VKContext::SetViewports(int count, Viewport *viewports) {

diff --git a/GPU/Common/GPUStateUtils.h b/GPU/Common/GPUStateUtils.h
@@ -63,7 +63,6 @@ ReplaceBlendType ReplaceBlendWithShader(bool allowShaderBlend, GEBufferFormat bu
 
 LogicOpReplaceType ReplaceLogicOpType();
 
-
 // Common representation, should be able to set this directly with any modern API.
 struct ViewportAndScissor {
 	bool scissorEnable;

diff --git a/GPU/Vulkan/StateMappingVulkan.cpp b/GPU/Vulkan/StateMappingVulkan.cpp
@@ -351,17 +351,17 @@ void DrawEngineVulkan::ConvertStateToVulkanKey(FramebufferManagerVulkan &fbManag
 			gstate_c.Dirty(DIRTY_PROJMATRIX);
 		}
 
-		VkRect2D &scissor = dynState.scissor;
+		ScissorRect &scissor = dynState.scissor;
 		if (vpAndScissor.scissorEnable) {
-			scissor.offset.x = vpAndScissor.scissorX;
-			scissor.offset.y = vpAndScissor.scissorY;
-			scissor.extent.width = std::max(0, vpAndScissor.scissorW);
-			scissor.extent.height = std::max(0, vpAndScissor.scissorH);
+			scissor.x = vpAndScissor.scissorX;
+			scissor.y = vpAndScissor.scissorY;
+			scissor.width = std::max(0, vpAndScissor.scissorW);
+			scissor.height = std::max(0, vpAndScissor.scissorH);
 		} else {
-			scissor.offset.x = 0;
-			scissor.offset.y = 0;
-			scissor.extent.width = framebufferManager_->GetRenderWidth();
-			scissor.extent.height = framebufferManager_->GetRenderHeight();
+			scissor.x = 0;
+			scissor.y = 0;
+			scissor.width = framebufferManager_->GetRenderWidth();
+			scissor.height = framebufferManager_->GetRenderHeight();
 		}
 	}
 }
@@ -381,7 +381,7 @@ void DrawEngineVulkan::BindShaderBlendTex() {
 
 void DrawEngineVulkan::ApplyDrawStateLate(VulkanRenderManager *renderManager, bool applyStencilRef, uint8_t stencilRef, bool useBlendConstant) {
 	if (gstate_c.IsDirty(DIRTY_VIEWPORTSCISSOR_STATE)) {
-		renderManager->SetScissor(dynState_.scissor);
+		renderManager->SetScissor(dynState_.scissor.x, dynState_.scissor.y, dynState_.scissor.width, dynState_.scissor.height);
 		renderManager->SetViewport(dynState_.viewport);
 	}
 	if ((gstate_c.IsDirty(DIRTY_DEPTHSTENCIL_STATE) && dynState_.useStencil) || applyStencilRef) {

diff --git a/GPU/Vulkan/StateMappingVulkan.h b/GPU/Vulkan/StateMappingVulkan.h
@@ -6,9 +6,14 @@
 
 class FramebufferManagerVulkan;
 
+struct ScissorRect {
+	int x, y;
+	int width, height;
+};
+
 struct VulkanDynamicState {
 	VkViewport viewport;
-	VkRect2D scissor;
+	ScissorRect scissor;
 	bool useBlendColor;
 	uint32_t blendColor;
 	bool useStencil;

diff --git a/GPU/Vulkan/StencilBufferVulkan.cpp b/GPU/Vulkan/StencilBufferVulkan.cpp
@@ -183,7 +183,7 @@ bool FramebufferManagerVulkan::NotifyStencilUpload(u32 addr, int size, StencilUp
 	VkPipeline pipeline = vulkan2D_->GetPipeline(rp, stencilVs_, stencilFs_, false, Vulkan2D::VK2DDepthStencilMode::STENCIL_REPLACE_ALWAYS);
 	renderManager->BindPipeline(pipeline, PIPELINE_FLAG_USES_DEPTH_STENCIL);
 	renderManager->SetViewport({ 0.0f, 0.0f, (float)w, (float)h, 0.0f, 1.0f });
-	renderManager->SetScissor({ { 0, 0, },{ (uint32_t)w, (uint32_t)h } });
+	renderManager->SetScissor(0, 0, (int)w, (int)h);
 
 	draw_->BindTextures(0, 1, &tex);
 	VkImageView drawPixelsImageView = (VkImageView)draw_->GetNativeObject(Draw::NativeObject::BOUND_TEXTURE0_IMAGEVIEW);

diff --git a/GPU/Vulkan/TextureCacheVulkan.cpp b/GPU/Vulkan/TextureCacheVulkan.cpp
@@ -524,7 +524,7 @@ void TextureCacheVulkan::ApplyTextureFramebuffer(VirtualFramebuffer *framebuffer
 			push.z_offset = scaleFactors.offset;
 			renderManager->PushConstants(vulkan2D_->GetPipelineLayout(), VK_SHADER_STAGE_VERTEX_BIT | VK_SHADER_STAGE_FRAGMENT_BIT, 0, sizeof(DepthPushConstants), &push);
 		}
-		renderManager->SetScissor(VkRect2D{ {0, 0}, { framebuffer->renderWidth, framebuffer->renderHeight} });
+		renderManager->SetScissor(0, 0, (int)framebuffer->renderWidth, (int)framebuffer->renderHeight);
 		renderManager->SetViewport(VkViewport{ 0.f, 0.f, (float)framebuffer->renderWidth, (float)framebuffer->renderHeight, 0.f, 1.f });
 		renderManager->Draw(vulkan2D_->GetPipelineLayout(), descSet, 0, nullptr, pushed, offset, 4);
 		shaderManagerVulkan_->DirtyLastShader();