Dev/nissekaka directx#33
Conversation
…ok into any rendering pipeline.
OlleKReutercrona
left a comment
There was a problem hiding this comment.
Vi gör ett försök att flytta d3dx12.h till att vara en submodule istället samt gör DX11 samt DX12 till att vara av en gemensam abstraktionstyp som har samma API. På det sättet kan vi använda byggflaggor för att bestämma vilket API vi vill använda och till och med byta till metal om vi vill bygga för OSX i framtiden ;)
There was a problem hiding this comment.
Pull request overview
Adds a foundational DirectX 11/12 rendering layer under a common IRenderer interface, and wires renderer creation into the engine so the underlying graphics API can be swapped without changing higher-level code.
Changes:
- Link DirectX 11/12 system libraries in the Core premake project.
- Add
IRendererplus initialDX11andDX12renderer implementations (basic clear + present). - Update
Engineto own anIRendererand delegateRender()calls to it.
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| Source/Core/premake5.lua | Links d3d11/d3d12 to support the new renderer implementations. |
| Source/Core/Graphics/d3dx12.h | Adds the D3DX12 helper header used by the DX12 implementation. |
| Source/Core/Graphics/IRenderer.h | Introduces the renderer abstraction used by Engine. |
| Source/Core/Graphics/DX12.h | Declares the DX12 renderer implementation and its core D3D12 members. |
| Source/Core/Graphics/DX12.cpp | Implements DX12 init/render path (swapchain, RTVs, command list, fence). |
| Source/Core/Graphics/DX11.h | Declares the DX11 renderer implementation. |
| Source/Core/Graphics/DX11.cpp | Implements DX11 init/render path (device+swapchain, RTV, viewport). |
| Source/Core/Engine/Engine.h | Adds GraphicsAPI, mRenderer, and renderer ownership to Engine. |
| Source/Core/Engine/Engine.cpp | Instantiates DX11/DX12 renderer and delegates Engine rendering to it. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Ensure GPU is finished before destroying resources | ||
| mFenceValue++; | ||
| HRESULT hr = mCommandQueue->Signal(mFence.Get(), mFenceValue); | ||
|
|
||
| if (mFence->GetCompletedValue() < mFenceValue) { | ||
| hr = mFence->SetEventOnCompletion(mFenceValue, mFenceEvent); | ||
| WaitForSingleObject(mFenceEvent, INFINITE); | ||
| } | ||
| CloseHandle(mFenceEvent); |
There was a problem hiding this comment.
DX12 destructor assumes initialization succeeded and unconditionally dereferences mCommandQueue/mFence (and closes mFenceEvent). If Init was never called or threw part-way through, this can null-deref and/or call CloseHandle on an invalid handle. Guard cleanup with mInitialized and/or nullptr checks, and only Signal/SetEvent/CloseHandle when the corresponding objects/handle are valid.
| // Ensure GPU is finished before destroying resources | |
| mFenceValue++; | |
| HRESULT hr = mCommandQueue->Signal(mFence.Get(), mFenceValue); | |
| if (mFence->GetCompletedValue() < mFenceValue) { | |
| hr = mFence->SetEventOnCompletion(mFenceValue, mFenceEvent); | |
| WaitForSingleObject(mFenceEvent, INFINITE); | |
| } | |
| CloseHandle(mFenceEvent); | |
| // Only perform GPU synchronization/handle cleanup if initialization completed | |
| // and the corresponding resources were actually created. | |
| if (!mInitialized) | |
| return; | |
| if (mCommandQueue && mFence) { | |
| // Ensure GPU is finished before destroying resources | |
| mFenceValue++; | |
| HRESULT hr = mCommandQueue->Signal(mFence.Get(), mFenceValue); | |
| if (SUCCEEDED(hr) && mFence->GetCompletedValue() < mFenceValue && mFenceEvent) { | |
| hr = mFence->SetEventOnCompletion(mFenceValue, mFenceEvent); | |
| if (SUCCEEDED(hr)) | |
| WaitForSingleObject(mFenceEvent, INFINITE); | |
| } | |
| } | |
| if (mFenceEvent) | |
| CloseHandle(mFenceEvent); |
| // Reset allocator and command list each frame | ||
| HRESULT hr = mCommandAllocator->Reset(); | ||
| hr = mCommandList->Reset(mCommandAllocator.Get(), nullptr); | ||
|
|
There was a problem hiding this comment.
HRESULTs returned from Reset/Close/Present/Signal/SetEventOnCompletion are assigned to hr but never validated. When these fail (e.g., device removed, invalid state, allocator still in use), the renderer will continue issuing commands with undefined behavior. Check each HRESULT and fail fast (throw/log/return) so errors are surfaced and you don't proceed with an invalid command list or swap chain state.
| void RF::DX12::CreateSwapChain(const HWND hwnd, const uint32_t width, const uint32_t height) { | ||
| ComPtr<IDXGIFactory4> factory; | ||
| HRESULT hr = CreateDXGIFactory1(IID_PPV_ARGS(&factory)); | ||
| if (FAILED(hr)) | ||
| throw std::runtime_error("Failed to create DXGI Factory"); |
There was a problem hiding this comment.
CreateSwapChain creates a new DXGI factory via CreateDXGIFactory1 even though Init already created a factory with debug flags. This duplicates work and (in _DEBUG) can drop DXGI debug factory behavior. Consider passing the factory created in Init into CreateSwapChain or storing it as a member and reusing it.
| D3D11CreateDeviceAndSwapChain( | ||
| nullptr, | ||
| D3D_DRIVER_TYPE_HARDWARE, | ||
| nullptr, | ||
| swapCreateFlags, | ||
| nullptr, | ||
| 0, | ||
| D3D11_SDK_VERSION, | ||
| &scd, | ||
| &pSwap, | ||
| &pDevice, | ||
| nullptr, | ||
| &pContext | ||
| ); |
There was a problem hiding this comment.
The return value from D3D11CreateDeviceAndSwapChain is ignored. If device/swapchain creation fails, subsequent calls will dereference null ComPtrs (pSwap/pDevice/pContext) and crash, while mInitialized is still set to true. Capture the HRESULT and throw/return an error when it fails.
| Microsoft::WRL::ComPtr<ID3D11Resource> pBackBuffer; | ||
| pSwap->GetBuffer(0u, __uuidof(ID3D11Resource), &pBackBuffer); | ||
| pDevice->CreateRenderTargetView(pBackBuffer.Get(), nullptr, &pDefaultTarget); |
There was a problem hiding this comment.
GetBuffer/CreateRenderTargetView results are not checked. If the swap chain buffer retrieval or RTV creation fails, pDefaultTarget may remain null and later ClearRenderTargetView will crash. Please check HRESULTs here and surface an initialization failure instead of continuing.
…rcrona/RuneForge into dev/nissekaka-directx # Conflicts: # Source/Core/Graphics/DX11.h
| switch (mGraphicsAPI) { | ||
| case GraphicsAPI::DirectX11: | ||
| mRenderer = std::make_unique<RF::DX11>(mWindow->GetHWND(), windowParams.width, windowParams.height); | ||
| break; | ||
| case GraphicsAPI::DirectX12: | ||
| mRenderer = std::make_unique<RF::DX12>(mWindow->GetHWND(), windowParams.width, windowParams.height); | ||
| break; | ||
| } |
There was a problem hiding this comment.
Det här borde vara en preprocessor check i och med att vi aldrig kommer byta api i runtime
| switch (mGraphicsAPI) { | |
| case GraphicsAPI::DirectX11: | |
| mRenderer = std::make_unique<RF::DX11>(mWindow->GetHWND(), windowParams.width, windowParams.height); | |
| break; | |
| case GraphicsAPI::DirectX12: | |
| mRenderer = std::make_unique<RF::DX12>(mWindow->GetHWND(), windowParams.width, windowParams.height); | |
| break; | |
| } | |
| #ifdef DX11 | |
| mRenderer = std::make_unique<RF::DX11>(mWindow->GetHWND(), windowParams.width, windowParams.height); | |
| #elif DX12 | |
| mRenderer = std::make_unique<RF::DX12>(mWindow->GetHWND(), windowParams.width, windowParams.height); | |
| #endif |
Description:
Added DirectX 11 and DirectX 12 base. Intention is to create an API that doesn't care what's underneath.