refactor: extract viewport object id picker

editor/src/Viewport/ViewportHostService.h

@@ -6,6 +6,7 @@
 #include "IViewportHostService.h"
 #include "SceneViewportPicker.h"
 #include "SceneViewportCameraController.h"
+#include "ViewportObjectIdPicker.h"
 #include "ViewportHostSurfaceUtils.h"
 #include "UI/ImGuiBackendBridge.h"
 
@@ -15,7 +16,6 @@
 #include <XCEngine/RHI/RHIEnums.h>
 #include <XCEngine/RHI/RHIResourceView.h>
 #include <XCEngine/RHI/RHITexture.h>
-#include <XCEngine/Rendering/ObjectIdEncoding.h>
 #include <XCEngine/Rendering/Passes/BuiltinInfiniteGridPass.h>
 #include <XCEngine/Rendering/Passes/BuiltinSceneViewPostPassPlan.h>
 #include <XCEngine/Rendering/Passes/BuiltinObjectIdOutlinePass.h>
@@ -876,46 +876,34 @@ private:
         const ImVec2& viewportSize,
         const ImVec2& viewportMousePosition,
         uint64_t& outEntityId) {
+        if (m_device == nullptr) {
             outEntityId = 0;
-
-        if (m_device == nullptr ||
-            m_sceneViewLastRenderContext.commandQueue == nullptr ||
-            entry.objectIdTexture == nullptr ||
-            !entry.hasValidObjectIdFrame ||
-            viewportSize.x <= 1.0f ||
-            viewportSize.y <= 1.0f ||
-            viewportMousePosition.x < 0.0f ||
-            viewportMousePosition.y < 0.0f ||
-            viewportMousePosition.x > viewportSize.x ||
-            viewportMousePosition.y > viewportSize.y) {
             return false;
         }
 
-        auto* commandQueue =
+        ViewportObjectIdPickContext pickContext = {};
-            m_sceneViewLastRenderContext.commandQueue;
+        pickContext.commandQueue = m_sceneViewLastRenderContext.commandQueue;
-        if (commandQueue == nullptr) {
+        pickContext.texture = entry.objectIdTexture;
-            return false;
+        pickContext.textureState = entry.objectIdState;
-        }
+        pickContext.textureWidth = entry.width;
+        pickContext.textureHeight = entry.height;
+        pickContext.hasValidFrame = entry.hasValidObjectIdFrame;
+        pickContext.viewportSize = viewportSize;
+        pickContext.viewportMousePosition = viewportMousePosition;
 
-        const uint32_t pixelX = ClampViewportPixelCoordinate(viewportMousePosition.x, entry.width);
+        return TryPickViewportObjectIdEntity(
-        const uint32_t pixelY = ClampViewportPixelCoordinate(viewportMousePosition.y, entry.height);
+            pickContext,
-        std::array<uint8_t, 4> rgba = {};
+            [this](const ViewportObjectIdReadbackRequest& request, std::array<uint8_t, 4>& outRgba) {
-        if (!m_device->ReadTexturePixelRGBA8(
+                return m_device != nullptr &&
-                commandQueue,
+                    m_device->ReadTexturePixelRGBA8(
-                entry.objectIdTexture,
+                        request.commandQueue,
-                entry.objectIdState,
+                        request.texture,
-                pixelX,
+                        request.textureState,
-                pixelY,
+                        request.pixelX,
-                rgba)) {
+                        request.pixelY,
-            return false;
+                        outRgba);
-        }
+            },
-
+            outEntityId);
-        outEntityId = static_cast<uint64_t>(Rendering::DecodeObjectIdFromColor(
-            rgba[0],
-            rgba[1],
-            rgba[2],
-            rgba[3]));
-        return true;
     }
 
     void DestroyViewportResources(ViewportEntry& entry) {

editor/src/Viewport/ViewportObjectIdPicker.h

@@ -0,0 +1,95 @@
+#pragma once
+
+#include "ViewportHostSurfaceUtils.h"
+
+#include <XCEngine/RHI/RHICommandQueue.h>
+#include <XCEngine/RHI/RHIEnums.h>
+#include <XCEngine/RHI/RHITexture.h>
+#include <XCEngine/Rendering/ObjectIdEncoding.h>
+
+#include <array>
+#include <cstdint>
+
+namespace XCEngine {
+namespace Editor {
+
+struct ViewportObjectIdPickContext {
+    RHI::RHICommandQueue* commandQueue = nullptr;
+    RHI::RHITexture* texture = nullptr;
+    RHI::ResourceStates textureState = RHI::ResourceStates::Common;
+    uint32_t textureWidth = 0;
+    uint32_t textureHeight = 0;
+    bool hasValidFrame = false;
+    ImVec2 viewportSize = ImVec2(0.0f, 0.0f);
+    ImVec2 viewportMousePosition = ImVec2(0.0f, 0.0f);
+};
+
+struct ViewportObjectIdReadbackRequest {
+    RHI::RHICommandQueue* commandQueue = nullptr;
+    RHI::RHITexture* texture = nullptr;
+    RHI::ResourceStates textureState = RHI::ResourceStates::Common;
+    uint32_t pixelX = 0;
+    uint32_t pixelY = 0;
+};
+
+inline bool CanPickViewportObjectId(const ViewportObjectIdPickContext& context) {
+    return context.commandQueue != nullptr &&
+        context.texture != nullptr &&
+        context.textureWidth > 0 &&
+        context.textureHeight > 0 &&
+        context.hasValidFrame &&
+        context.viewportSize.x > 1.0f &&
+        context.viewportSize.y > 1.0f &&
+        context.viewportMousePosition.x >= 0.0f &&
+        context.viewportMousePosition.y >= 0.0f &&
+        context.viewportMousePosition.x <= context.viewportSize.x &&
+        context.viewportMousePosition.y <= context.viewportSize.y;
+}
+
+inline bool BuildViewportObjectIdReadbackRequest(
+    const ViewportObjectIdPickContext& context,
+    ViewportObjectIdReadbackRequest& outRequest) {
+    outRequest = {};
+    if (!CanPickViewportObjectId(context)) {
+        return false;
+    }
+
+    outRequest.commandQueue = context.commandQueue;
+    outRequest.texture = context.texture;
+    outRequest.textureState = context.textureState;
+    outRequest.pixelX = ClampViewportPixelCoordinate(
+        context.viewportMousePosition.x,
+        context.textureWidth);
+    outRequest.pixelY = ClampViewportPixelCoordinate(
+        context.viewportMousePosition.y,
+        context.textureHeight);
+    return true;
+}
+
+template <typename ReadPixelFn>
+bool TryPickViewportObjectIdEntity(
+    const ViewportObjectIdPickContext& context,
+    ReadPixelFn&& readPixel,
+    uint64_t& outEntityId) {
+    outEntityId = 0;
+
+    ViewportObjectIdReadbackRequest request = {};
+    if (!BuildViewportObjectIdReadbackRequest(context, request)) {
+        return false;
+    }
+
+    std::array<uint8_t, 4> rgba = {};
+    if (!readPixel(request, rgba)) {
+        return false;
+    }
+
+    outEntityId = static_cast<uint64_t>(Rendering::DecodeObjectIdFromColor(
+        rgba[0],
+        rgba[1],
+        rgba[2],
+        rgba[3]));
+    return true;
+}
+
+} // namespace Editor
+} // namespace XCEngine

tests/editor/CMakeLists.txt

@@ -11,6 +11,7 @@ set(EDITOR_TEST_SOURCES
     test_scene_viewport_picker.cpp
     test_scene_viewport_overlay_renderer.cpp
     test_viewport_host_surface_utils.cpp
+    test_viewport_object_id_picker.cpp
     test_builtin_icon_layout_utils.cpp
     ${CMAKE_SOURCE_DIR}/editor/src/Core/UndoManager.cpp
     ${CMAKE_SOURCE_DIR}/editor/src/Managers/SceneManager.cpp

tests/editor/test_viewport_object_id_picker.cpp

@@ -0,0 +1,115 @@
+#include <gtest/gtest.h>
+
+#include "Viewport/ViewportObjectIdPicker.h"
+
+#include <cstdint>
+
+namespace {
+
+using XCEngine::Editor::BuildViewportObjectIdReadbackRequest;
+using XCEngine::Editor::CanPickViewportObjectId;
+using XCEngine::Editor::TryPickViewportObjectIdEntity;
+using XCEngine::Editor::ViewportObjectIdPickContext;
+using XCEngine::Editor::ViewportObjectIdReadbackRequest;
+using XCEngine::RHI::RHICommandQueue;
+using XCEngine::RHI::RHITexture;
+using XCEngine::RHI::ResourceStates;
+
+RHICommandQueue* MakeDummyQueue() {
+    return reinterpret_cast<RHICommandQueue*>(static_cast<uintptr_t>(0x1));
+}
+
+RHITexture* MakeDummyTexture() {
+    return reinterpret_cast<RHITexture*>(static_cast<uintptr_t>(0x2));
+}
+
+ViewportObjectIdPickContext CreateValidContext() {
+    ViewportObjectIdPickContext context = {};
+    context.commandQueue = MakeDummyQueue();
+    context.texture = MakeDummyTexture();
+    context.textureState = ResourceStates::PixelShaderResource;
+    context.textureWidth = 1280;
+    context.textureHeight = 720;
+    context.hasValidFrame = true;
+    context.viewportSize = ImVec2(1280.0f, 720.0f);
+    context.viewportMousePosition = ImVec2(640.0f, 360.0f);
+    return context;
+}
+
+TEST(ViewportObjectIdPickerTest, CanPickRejectsMissingOrOutOfBoundsInputs) {
+    ViewportObjectIdPickContext context = CreateValidContext();
+    EXPECT_TRUE(CanPickViewportObjectId(context));
+
+    context.commandQueue = nullptr;
+    EXPECT_FALSE(CanPickViewportObjectId(context));
+
+    context = CreateValidContext();
+    context.texture = nullptr;
+    EXPECT_FALSE(CanPickViewportObjectId(context));
+
+    context = CreateValidContext();
+    context.hasValidFrame = false;
+    EXPECT_FALSE(CanPickViewportObjectId(context));
+
+    context = CreateValidContext();
+    context.textureWidth = 0;
+    EXPECT_FALSE(CanPickViewportObjectId(context));
+
+    context = CreateValidContext();
+    context.viewportMousePosition = ImVec2(-1.0f, 10.0f);
+    EXPECT_FALSE(CanPickViewportObjectId(context));
+
+    context = CreateValidContext();
+    context.viewportMousePosition = ImVec2(10.0f, 721.0f);
+    EXPECT_FALSE(CanPickViewportObjectId(context));
+}
+
+TEST(ViewportObjectIdPickerTest, BuildReadbackRequestMapsViewportCoordinatesToTexturePixels) {
+    ViewportObjectIdPickContext context = CreateValidContext();
+    context.viewportMousePosition = ImVec2(1280.0f, 720.0f);
+
+    ViewportObjectIdReadbackRequest request = {};
+    ASSERT_TRUE(BuildViewportObjectIdReadbackRequest(context, request));
+    EXPECT_EQ(request.commandQueue, context.commandQueue);
+    EXPECT_EQ(request.texture, context.texture);
+    EXPECT_EQ(request.textureState, ResourceStates::PixelShaderResource);
+    EXPECT_EQ(request.pixelX, 1279u);
+    EXPECT_EQ(request.pixelY, 719u);
+}
+
+TEST(ViewportObjectIdPickerTest, TryPickViewportObjectIdEntityDecodesReadbackColor) {
+    const ViewportObjectIdPickContext context = CreateValidContext();
+
+    uint64_t entityId = 0;
+    ViewportObjectIdReadbackRequest capturedRequest = {};
+    const bool picked = TryPickViewportObjectIdEntity(
+        context,
+        [&capturedRequest](const ViewportObjectIdReadbackRequest& request, std::array<uint8_t, 4>& outRgba) {
+            capturedRequest = request;
+            outRgba = { 0x78, 0x56, 0x34, 0x12 };
+            return true;
+        },
+        entityId);
+
+    EXPECT_TRUE(picked);
+    EXPECT_EQ(entityId, 0x12345678ull);
+    EXPECT_EQ(capturedRequest.pixelX, 640u);
+    EXPECT_EQ(capturedRequest.pixelY, 360u);
+}
+
+TEST(ViewportObjectIdPickerTest, TryPickViewportObjectIdEntityFailsWhenReadbackFails) {
+    const ViewportObjectIdPickContext context = CreateValidContext();
+
+    uint64_t entityId = 99;
+    const bool picked = TryPickViewportObjectIdEntity(
+        context,
+        [](const ViewportObjectIdReadbackRequest&, std::array<uint8_t, 4>&) {
+            return false;
+        },
+        entityId);
+
+    EXPECT_FALSE(picked);
+    EXPECT_EQ(entityId, 0u);
+}
+
+} // namespace