feat(options): Implement game options for texture filter mode, anisotropy and MSAA selection#2482
Conversation
|
| Filename | Overview |
|---|---|
| Core/GameEngine/Include/Common/OptionPreferences.h | Adds AntiAliasingMode enum and three new getter declarations; now includes rendering-layer headers (ww3d.h, texturefilter.h) in a widely-included core common header, creating a layering concern. |
| Core/Libraries/Source/WWVegas/WW3D2/texturefilter.cpp | Adds TextureFilterModeString lookup table and getTextureFilterMode() helper (falls back to TEXTURE_FILTER_NONE on mismatch, inconsistent with the BILINEAR default used elsewhere), and updates _Init_Filters signature to accept anisotropy level. |
| Core/Libraries/Include/Lib/BaseType.h | Adds highestBit() template that rounds a value down to the nearest power of two; correct for both signed and unsigned inputs with appropriate static_assert guard. |
| Generals/Code/GameEngine/Include/Common/GlobalData.h | Replaces m_antiAliasBoxValue with m_antiAliasLevel and adds m_textureFilteringMode and m_textureAnisotropyLevel; comment alignment across the three fields is inconsistent. |
| Generals/Code/GameEngineDevice/Source/W3DDevice/GameClient/W3DDisplay.cpp | Sets MSAA mode from global data before render device creation, then applies texture filter and anisotropy settings only on WW3D_ERROR_OK — correctly guarded against failed device creation. |
| Generals/Code/Libraries/Source/WWVegas/WW3D2/ww3d.cpp | Adds AnisotropyLevel static, Set_Anisotropy_Level() using clamp+highestBit, and updates Set_Texture_Filter() to pass current AnisotropyLevel to _Init_Filters; logic and clamping are correct. |
Sequence Diagram
sequenceDiagram
participant INI as Options.ini
participant OP as OptionPreferences
participant GD as GlobalData
participant W3DD as W3DDisplay::init()
participant WW3D as WW3D
participant TF as TextureFilterClass
INI->>OP: load("Options.ini")
OP->>GD: parseGameDataDefinition()
W3DD->>WW3D: Set_MSAA_Mode(m_antiAliasLevel)
W3DD->>WW3D: Set_Render_Device(...)
WW3D->>TF: _Init_Filters(TextureFilter, AnisotropyLevel)
alt renderDeviceError == WW3D_ERROR_OK
W3DD->>WW3D: Set_Texture_Filter(m_textureFilteringMode)
WW3D->>TF: _Init_Filters(TextureFilter, AnisotropyLevel)
W3DD->>WW3D: Set_Anisotropy_Level(m_textureAnisotropyLevel)
WW3D->>TF: _Set_Max_Anisotropy(AnisotropyLevel)
end
Prompt To Fix All With AI
This is a comment left during a code review.
Path: Core/Libraries/Source/WWVegas/WW3D2/texturefilter.cpp
Line: 148
Comment:
**Invalid string falls back to `TEXTURE_FILTER_NONE`, not `TEXTURE_FILTER_BILINEAR`**
When `getTextureFilterMode()` receives an unrecognized string (e.g. a typo like `"Billinear"`), it returns `TEXTURE_FILTER_NONE`, but `OptionPreferences::getTextureFilterMode()` returns `TEXTURE_FILTER_BILINEAR` when the `TextureFilter` key is entirely absent. This means a misconfigured INI key silently degrades the user to *no* texture filtering — worse than the intended default — rather than falling back gracefully to bilinear.
Consider returning the intended default (`TEXTURE_FILTER_BILINEAR`) from the static helper when no match is found:
```cpp
return TextureFilterClass::TEXTURE_FILTER_BILINEAR;
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: Core/GameEngine/Include/Common/OptionPreferences.h
Line: 9-10
Comment:
**Rendering-layer headers pulled into a core common header**
Including `ww3d.h` and `texturefilter.h` directly in `OptionPreferences.h` creates a hard dependency from the core game engine common layer on the rendering library. Any translation unit that transitively includes `OptionPreferences.h` (which is widely used) will now also pull in all rendering headers and their transitive dependencies, which may cause compilation issues in translation units that don't have the WW3D include paths configured, and makes the core layer non-portable.
Consider forward-declaring the return types (or using `Int`/`UnsignedInt` as the public API and casting internally inside the `.cpp` file where the rendering headers are already included), keeping `OptionPreferences.h` free of graphics-layer includes.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: Generals/Code/GameEngine/Include/Common/GlobalData.h
Line: 400-402
Comment:
**Comment columns not aligned across the three new fields**
The `///<` comment delimiters on the three new fields are not column-aligned: `m_antiAliasLevel` has 10 spaces before the comment, `m_textureFilteringMode` has 7, and `m_textureAnisotropyLevel` has 5. Per the codebase's data-block alignment convention these should be aligned to the same column. The same pattern applies to the parallel block in `GeneralsMD/Code/GameEngine/Include/Common/GlobalData.h`.
```cpp
UnsignedInt m_antiAliasLevel; ///< value of selected antialias level in the game options
UnsignedInt m_textureFilteringMode; ///< value related to TextureFilterClass::TextureFilterModeEnum
UnsignedInt m_textureAnisotropyLevel; ///< value related to TextureFilterClass::AnisotropicFilterMode
```
**Rule Used:** Align columns in data-oriented blocks (like lookup... ([source](https://app.greptile.com/review/custom-context?memory=e61e83cf-a14f-4759-bed8-3bab008b25b5))
**Learned From**
[TheSuperHackers/GeneralsGameCode#2067](https://github.com/TheSuperHackers/GeneralsGameCode/pull/2067#discussion_r2704192059)
How can I resolve this? If you propose a fix, please make it concise.Reviews (15): Last reviewed commit: "feat(options): Implement game options fo..." | Re-trigger Greptile
120d3db to
cfff7d1
Compare
|
Generals is currently broken till i replicate across due to changes in initaialising the texture filtering |
915e548 to
53085fc
Compare
xezon
left a comment
There was a problem hiding this comment.
This implementation can be polished more.
e222f4b to
09fc9af
Compare
09fc9af to
a4f900f
Compare
|
Updated based on feedback |
xezon
left a comment
There was a problem hiding this comment.
There is a lot of casting from enum type to int and back to enum type. Why is that like so? Can we not stick to enum types for all these integers? And if not, then what is the point of the enum types if we need to use integers?
|
|
||
| // TheSuperHackers @info Update the MSAA mode that was set as some GPU's may not support certain levels | ||
| // Texture filtering must be also be updated after render device initialisation | ||
| TheWritableGlobalData->m_antiAliasLevel = (UnsignedInt)WW3D::Get_MSAA_Mode(); |
There was a problem hiding this comment.
Can we make m_antiAliasLevel that enum type?
a4f900f to
67556b4
Compare
|
Tweaked based on recent Feedback |
67556b4 to
038928f
Compare
|
Tweaked based on majority of feedback |
038928f to
c360c0e
Compare
|
Updated based on feedback, just need to fix merge conflicts |
c360c0e to
ecf7bfd
Compare
|
Rebased with main, but main changes were based on last two review comments in
|
|
Had to add an extra fix commit to get the builds working in release again, no idea why it would now alert about the unused variable, unless the error was originaly being supressed. |
b8c720d to
75dcb4c
Compare
|
Updated, if good to go i will replicate across to generals. I also re-ordered the commits so the one fixing the unused variable is first. |
xezon
left a comment
There was a problem hiding this comment.
Can the first commit be a separate pull then? It looks to be not related to game options.
It's not related but was required to allow the PR to build in release. I seperated it out into #2629 now. This PR will start failing to build in zero hour release again once i update it. |
75dcb4c to
8d740e1
Compare
|
Removed build fix commit, PR will currently fail to build zero hour release builds untill the build fix is merged to main and this PR is rebased. |
| // TheSuperHackers @info Update the MSAA mode that was set as some GPU's may not support certain levels | ||
| // Texture filtering must be also be updated after render device initialisation | ||
| TheWritableGlobalData->m_antiAliasLevel = (UnsignedInt)WW3D::Get_MSAA_Mode(); | ||
| WW3D::Set_Texture_Filter(TheWritableGlobalData->m_textureFilteringMode); | ||
| TheWritableGlobalData->m_textureFilteringMode = WW3D::Get_Texture_Filter(); | ||
| WW3D::Set_Anisotropy_Level(TheWritableGlobalData->m_textureAnisotropyLevel); | ||
| TheWritableGlobalData->m_textureAnisotropyLevel = WW3D::Get_Anisotropy_Level(); |
There was a problem hiding this comment.
Texture filter init called inside retry loop when device may not be ready
WW3D::Set_Texture_Filter (and Set_Anisotropy_Level) are called unconditionally after Set_Render_Device, even if it returned an error. Set_Texture_Filter internally calls TextureFilterClass::_Init_Filters, which immediately dereferences DX8Wrapper::Get_Current_Caps()->Get_DX8_Caps(). If device creation failed on the first retry attempt, the caps pointer may be null or stale, leading to a crash or undefined behaviour before the second retry has a chance to run.
Guard these calls so they only execute on a successful device creation:
TheWritableGlobalData->m_antiAliasLevel = (UnsignedInt)WW3D::Get_MSAA_Mode();
if (renderDeviceError == WW3D_ERROR_OK)
{
WW3D::Set_Texture_Filter(TheWritableGlobalData->m_textureFilteringMode);
TheWritableGlobalData->m_textureFilteringMode = WW3D::Get_Texture_Filter();
WW3D::Set_Anisotropy_Level(TheWritableGlobalData->m_textureAnisotropyLevel);
TheWritableGlobalData->m_textureAnisotropyLevel = WW3D::Get_Anisotropy_Level();
}Prompt To Fix With AI
This is a comment left during a code review.
Path: GeneralsMD/Code/GameEngineDevice/Source/W3DDevice/GameClient/W3DDisplay.cpp
Line: 780-786
Comment:
**Texture filter init called inside retry loop when device may not be ready**
`WW3D::Set_Texture_Filter` (and `Set_Anisotropy_Level`) are called unconditionally after `Set_Render_Device`, even if it returned an error. `Set_Texture_Filter` internally calls `TextureFilterClass::_Init_Filters`, which immediately dereferences `DX8Wrapper::Get_Current_Caps()->Get_DX8_Caps()`. If device creation failed on the first retry attempt, the caps pointer may be null or stale, leading to a crash or undefined behaviour before the second retry has a chance to run.
Guard these calls so they only execute on a successful device creation:
```cpp
TheWritableGlobalData->m_antiAliasLevel = (UnsignedInt)WW3D::Get_MSAA_Mode();
if (renderDeviceError == WW3D_ERROR_OK)
{
WW3D::Set_Texture_Filter(TheWritableGlobalData->m_textureFilteringMode);
TheWritableGlobalData->m_textureFilteringMode = WW3D::Get_Texture_Filter();
WW3D::Set_Anisotropy_Level(TheWritableGlobalData->m_textureAnisotropyLevel);
TheWritableGlobalData->m_textureAnisotropyLevel = WW3D::Get_Anisotropy_Level();
}
```
How can I resolve this? If you propose a fix, please make it concise.8d740e1 to
31a0475
Compare
|
Rebased on main and fixed initialisation issue brought up by greptile If everything is good i will replicate to generals. |
31a0475 to
8f3536c
Compare
|
Replicated to generals |
|
Greptile shows 2 P2 findings. |
The The |
…sotropy level (#2482)
8f3536c to
a408f8d
Compare
|
tweaked to use a clamp instead of the two if statements, this should be good to go now. |
Merge By Rebase
Merge after #2629
Closes: #2474
Closes: #2375
This PR implements options for allowing the enablement of MSAA and the selection of different texture filtering modes and setting the anisotropic filtering level.
The following options are added for anti aliasing
AntiAliasing = 0, 2, 4, 8
For now the numbers relate to the MSAA sample level and Zero disables MSAA as we only support MSAA at the moment.
TextureFilter = None, Point, Bilinear, Trilinear, Anisotropic
Self explanatory, just choose one and see how the game looks
AnisotropyLevel = 2, 4, 8, 16
The anisotropy level only comes into play when Anisotropic filtering is set. 2 is also the minimum value.
Generals is broken untill the changes are replicated due to changes in texture filter init.