Restores the original blend config after a surface render. - #7
Conversation
There was a problem hiding this comment.
Code Review
This pull request implements state tracking for blending configurations in NV2AState to ensure the state is correctly restored after rendering to non-framebuffer surfaces. It also adds environment variable configuration for NXDK_DIR in CMake. The review feedback highlights a redundant and potentially incorrect variable assignment in CMakeLists.txt, suggests using default member initializers for the new BlendConfig struct for better safety, and recommends maintaining the const qualifier on SetBlend methods by marking the tracking members as mutable to avoid breaking the existing API.
| struct BlendConfig { | ||
| bool enable; | ||
| uint32_t func; | ||
| uint32_t sfactor; | ||
| uint32_t dfactor; | ||
| }; |
There was a problem hiding this comment.
It is safer to provide default member initializers for the BlendConfig struct that match the hardware defaults. This ensures that if RenderToSurfaceStart is called before any explicit call to SetBlend, the state can be correctly restored to the expected defaults rather than zero-initialized values (which may not correspond to the intended hardware state).
| struct BlendConfig { | |
| bool enable; | |
| uint32_t func; | |
| uint32_t sfactor; | |
| uint32_t dfactor; | |
| }; | |
| struct BlendConfig { | |
| bool enable = false; | |
| uint32_t func = NV097_SET_BLEND_EQUATION_V_FUNC_ADD; | |
| uint32_t sfactor = NV097_SET_BLEND_FUNC_SFACTOR_V_SRC_ALPHA; | |
| uint32_t dfactor = NV097_SET_BLEND_FUNC_DFACTOR_V_ONE_MINUS_SRC_ALPHA; | |
| }; |
| block(SCOPE_FOR VARIABLES) | ||
| get_filename_component(NXDK_ROOT_DIR "${CMAKE_TOOLCHAIN_FILE}/../.." ABSOLUTE) | ||
| set(NXDK_DIR "${NXDK_ROOT_DIR}" CACHE PATH "Path to the nxdk root directory.") | ||
| set(ENV{NXDK_DIR} "${NXDK_SOURCE_DIR}") |
There was a problem hiding this comment.
The variable ${NXDK_SOURCE_DIR} is not defined at this point in the script; it is defined within PrebuildNXDK.cmake, which is included later on line 21. Furthermore, PrebuildNXDK.cmake already sets the NXDK_DIR environment variable, making this line redundant and potentially incorrect if it sets the environment variable to an empty string.
| void SetBlend(bool enable = true, uint32_t func = NV097_SET_BLEND_EQUATION_V_FUNC_ADD, | ||
| uint32_t sfactor = NV097_SET_BLEND_FUNC_SFACTOR_V_SRC_ALPHA, | ||
| uint32_t dfactor = NV097_SET_BLEND_FUNC_DFACTOR_V_ONE_MINUS_SRC_ALPHA) const; | ||
| uint32_t dfactor = NV097_SET_BLEND_FUNC_DFACTOR_V_ONE_MINUS_SRC_ALPHA); |
There was a problem hiding this comment.
Removing the const qualifier from SetBlend is a breaking change for any code that interacts with a const NV2AState object. Since the internal tracking of the blend configuration is essentially a cache of the hardware state, the tracking members should be marked mutable to allow this method to remain const.
| void SetBlend(bool enable = true, uint32_t func = NV097_SET_BLEND_EQUATION_V_FUNC_ADD, | |
| uint32_t sfactor = NV097_SET_BLEND_FUNC_SFACTOR_V_SRC_ALPHA, | |
| uint32_t dfactor = NV097_SET_BLEND_FUNC_DFACTOR_V_ONE_MINUS_SRC_ALPHA) const; | |
| uint32_t dfactor = NV097_SET_BLEND_FUNC_DFACTOR_V_ONE_MINUS_SRC_ALPHA); | |
| void SetBlend(bool enable = true, uint32_t func = NV097_SET_BLEND_EQUATION_V_FUNC_ADD, | |
| uint32_t sfactor = NV097_SET_BLEND_FUNC_SFACTOR_V_SRC_ALPHA, | |
| uint32_t dfactor = NV097_SET_BLEND_FUNC_DFACTOR_V_ONE_MINUS_SRC_ALPHA) const; |
| uint32_t dfactor = NV097_SET_BLEND_FUNC_DFACTOR_V_ONE_MINUS_SRC_ALPHA) const; | ||
| uint32_t dfactor = NV097_SET_BLEND_FUNC_DFACTOR_V_ONE_MINUS_SRC_ALPHA); | ||
|
|
||
| void SetBlend(const BlendConfig &config) { SetBlend(config.enable, config.func, config.sfactor, config.dfactor); } |
There was a problem hiding this comment.
This overload should also be const to match the primary SetBlend method.
| void SetBlend(const BlendConfig &config) { SetBlend(config.enable, config.func, config.sfactor, config.dfactor); } | |
| void SetBlend(const BlendConfig &config) const { SetBlend(config.enable, config.func, config.sfactor, config.dfactor); } |
| kNoStrideOverride, kNoStrideOverride, kNoStrideOverride, kNoStrideOverride, kNoStrideOverride, kNoStrideOverride, | ||
| kNoStrideOverride, kNoStrideOverride, kNoStrideOverride, kNoStrideOverride}; | ||
|
|
||
| BlendConfig active_blend_config_{.enable = false}; |
There was a problem hiding this comment.
Marking active_blend_config_ as mutable allows the SetBlend methods to remain const, preserving the existing API contract. Additionally, if default initializers are added to the BlendConfig struct, the explicit initialization here can be simplified.
| BlendConfig active_blend_config_{.enable = false}; | |
| mutable BlendConfig active_blend_config_; |
| } | ||
|
|
||
| void NV2AState::SetBlend(bool enable, uint32_t func, uint32_t sfactor, uint32_t dfactor) const { | ||
| void NV2AState::SetBlend(bool enable, uint32_t func, uint32_t sfactor, uint32_t dfactor) { |
486996e to
b6ac75a
Compare
No description provided.