Merge pull request #12923 from unknownbrackets/ui-reporting

Fix crash on report feedback screen

UI/ReportScreen.cpp

@@ -161,6 +161,28 @@ ReportScreen::ReportScreen(const std::string &gamePath)
 	ratingEnabled_ = enableReporting_;
 }
 
+void ReportScreen::postRender() {
+	// We do this after render because we need it to be within the frame (so the screenshot works).
+	// We could do it mid frame, but then we have to reapply viewport/scissor.
+	if (!tookScreenshot_) {
+		std::string path = GetSysDirectory(DIRECTORY_SCREENSHOT);
+		if (!File::Exists(path)) {
+			File::CreateDir(path);
+		}
+		screenshotFilename_ = path + ".reporting.jpg";
+		if (TakeGameScreenshot(screenshotFilename_.c_str(), ScreenshotFormat::JPG, SCREENSHOT_DISPLAY, nullptr, nullptr, 4)) {
+			// Redo the views already, now with a screenshot included.
+			RecreateViews();
+		} else {
+			// Good news (?), the views are good as-is without a screenshot.
+			screenshotFilename_.clear();
+		}
+		tookScreenshot_ = true;
+	}
+
+	UIDialogScreenWithGameBackground::postRender();
+}
+
 void ReportScreen::update() {
 	if (screenshot_) {
 		if (includeScreenshot_) {
@@ -252,18 +274,13 @@ void ReportScreen::CreateViews() {
 	}
 #endif
 
-	std::string path = GetSysDirectory(DIRECTORY_SCREENSHOT);
-	if (!File::Exists(path)) {
-		File::CreateDir(path);
-	}
-	screenshotFilename_ = path + ".reporting.jpg";
-	int shotWidth = 0, shotHeight = 0;
-	if (TakeGameScreenshot(screenshotFilename_.c_str(), ScreenshotFormat::JPG, SCREENSHOT_DISPLAY, &shotWidth, &shotHeight, 4)) {
-		float scale = 340.0f * (1.0f / g_dpi_scale_y) * (1.0f / shotHeight);
+	if (tookScreenshot_ && !screenshotFilename_.empty()) {
 		leftColumnItems->Add(new CheckBox(&includeScreenshot_, rp->T("FeedbackIncludeScreen", "Include a screenshot")))->SetEnabledPtr(&enableReporting_);
 		screenshot_ = leftColumnItems->Add(new AsyncImageFileView(screenshotFilename_, IS_KEEP_ASPECT, nullptr, new LinearLayoutParams(FILL_PARENT, WRAP_CONTENT, Margins(12, 0))));
 	} else {
-		includeScreenshot_ = false;
+		if (tookScreenshot_) {
+			includeScreenshot_ = false;
+		}
 		screenshot_ = nullptr;
 	}
 
@@ -333,7 +350,7 @@ EventReturn ReportScreen::HandleSubmit(EventParams &e) {
 		g_Config.Save("ReportScreen::HandleSubmit");
 	}
 
-	std::string filename = includeScreenshot_ ? screenshotFilename_ : "";
+	std::string filename = tookScreenshot_ && includeScreenshot_ ? screenshotFilename_ : "";
 	Reporting::ReportCompatibility(compat, graphics_ + 1, speed_ + 1, gameplay_ + 1, filename);
 	TriggerFinish(DR_OK);
 	screenManager()->push(new ReportFinishScreen(gamePath_, overall_));

UI/ReportScreen.h

@@ -37,6 +37,7 @@ public:
 	ReportScreen(const std::string &gamePath);
 
 protected:
+	void postRender() override;
 	void update() override;
 	void resized() override;
 	void CreateViews() override;
@@ -60,6 +61,7 @@ protected:
 	int gameplay_ = -1;
 	bool enableReporting_;
 	bool ratingEnabled_;
+	bool tookScreenshot_ = false;
 	bool includeScreenshot_ = true;
 };
 

ext/native/thin3d/GLRenderManager.cpp

@@ -269,6 +269,7 @@ void GLRenderManager::BindFramebufferAsRenderTarget(GLRFramebuffer *fb, GLRRende
 	if (steps_.size() && steps_.back()->render.framebuffer == fb && steps_.back()->stepType == GLRStepType::RENDER) {
 		if (color != GLRRenderPassAction::CLEAR && depth != GLRRenderPassAction::CLEAR && stencil != GLRRenderPassAction::CLEAR) {
 			// We don't move to a new step, this bind was unnecessary and we can safely skip it.
+			curRenderStep_ = steps_.back();
 			return;
 		}
 	}

ext/native/thin3d/VulkanRenderManager.cpp

@@ -461,6 +461,14 @@ void VulkanRenderManager::BindFramebufferAsRenderTarget(VKRFramebuffer *fb, VKRR
 		if (color != VKRRenderPassAction::CLEAR && depth != VKRRenderPassAction::CLEAR && stencil != VKRRenderPassAction::CLEAR) {
 			// We don't move to a new step, this bind was unnecessary and we can safely skip it.
 			// Not sure how much this is still happening but probably worth checking for nevertheless.
+			curRenderStep_ = steps_.back();
+			curStepHasViewport_ = false;
+			for (const auto &c : steps_.back()->commands) {
+				if (c.cmd == VKRRenderCommand::VIEWPORT) {
+					curStepHasViewport_ = true;
+					break;
+				}
+			}
 			return;
 		}
 	}
@@ -498,6 +506,7 @@ void VulkanRenderManager::BindFramebufferAsRenderTarget(VKRFramebuffer *fb, VKRR
 	}
 
 	curRenderStep_ = step;
+	curStepHasViewport_ = false;
 	if (fb) {
 		curWidth_ = fb->width;
 		curHeight_ = fb->height;
@@ -508,6 +517,7 @@ void VulkanRenderManager::BindFramebufferAsRenderTarget(VKRFramebuffer *fb, VKRR
 }
 
 bool VulkanRenderManager::CopyFramebufferToMemorySync(VKRFramebuffer *src, int aspectBits, int x, int y, int w, int h, Draw::DataFormat destFormat, uint8_t *pixels, int pixelStride) {
+	assert(insideFrame_);
 	for (int i = (int)steps_.size() - 1; i >= 0; i--) {
 		if (steps_[i]->stepType == VKRStepType::RENDER && steps_[i]->render.framebuffer == src) {
 			steps_[i]->render.numReads++;
@@ -568,6 +578,7 @@ bool VulkanRenderManager::CopyFramebufferToMemorySync(VKRFramebuffer *src, int a
 }
 
 void VulkanRenderManager::CopyImageToMemorySync(VkImage image, int mipLevel, int x, int y, int w, int h, Draw::DataFormat destFormat, uint8_t *pixels, int pixelStride) {
+	assert(insideFrame_);
 	VKRStep *step = new VKRStep{ VKRStepType::READBACK_IMAGE };
 	step->readback_image.image = image;
 	step->readback_image.srcRect.offset = { x, y };

ext/native/thin3d/VulkanRenderManager.h

@@ -136,6 +136,7 @@ public:
 		data.viewport.vp.minDepth = clamp_value(vp.minDepth, 0.0f, 1.0f);
 		data.viewport.vp.maxDepth = clamp_value(vp.maxDepth, 0.0f, 1.0f);
 		curRenderStep_->commands.push_back(data);
+		curStepHasViewport_ = true;
 	}
 
 	void SetScissor(const VkRect2D &rc) {
@@ -145,6 +146,7 @@ public:
 		VkRenderData data{ VKRRenderCommand::SCISSOR };
 		data.scissor.scissor = rc;
 		curRenderStep_->commands.push_back(data);
+		curStepHasScissor_ = true;
 	}
 
 	void SetStencilParams(uint8_t writeMask, uint8_t compareMask, uint8_t refValue) {
@@ -178,7 +180,7 @@ public:
 	void Clear(uint32_t clearColor, float clearZ, int clearStencil, int clearMask);
 
 	void Draw(VkPipelineLayout layout, VkDescriptorSet descSet, int numUboOffsets, const uint32_t *uboOffsets, VkBuffer vbuffer, int voffset, int count, int offset = 0) {
-		_dbg_assert_(G3D, curRenderStep_ && curRenderStep_->stepType == VKRStepType::RENDER);
+		_dbg_assert_(G3D, curRenderStep_ && curRenderStep_->stepType == VKRStepType::RENDER && curStepHasViewport_ && curStepHasScissor_);
 		VkRenderData data{ VKRRenderCommand::DRAW };
 		data.draw.count = count;
 		data.draw.offset = offset;
@@ -195,7 +197,7 @@ public:
 	}
 
 	void DrawIndexed(VkPipelineLayout layout, VkDescriptorSet descSet, int numUboOffsets, const uint32_t *uboOffsets, VkBuffer vbuffer, int voffset, VkBuffer ibuffer, int ioffset, int count, int numInstances, VkIndexType indexType) {
-		_dbg_assert_(G3D, curRenderStep_ && curRenderStep_->stepType == VKRStepType::RENDER);
+		_dbg_assert_(G3D, curRenderStep_ && curRenderStep_->stepType == VKRStepType::RENDER && curStepHasViewport_ && curStepHasScissor_);
 		VkRenderData data{ VKRRenderCommand::DRAW_INDEXED };
 		data.drawIndexed.count = count;
 		data.drawIndexed.instances = numInstances;
@@ -315,6 +317,8 @@ private:
 	int curHeight_ = -1;
 	bool insideFrame_ = false;
 	VKRStep *curRenderStep_ = nullptr;
+	bool curStepHasViewport_ = false;
+	bool curStepHasScissor_ = false;
 	std::vector<VKRStep *> steps_;
 	bool splitSubmit_ = false;