fix(view): Adjust default camera height to compensate for screen aspect ratio#1711
fix(view): Adjust default camera height to compensate for screen aspect ratio#1711Mauller wants to merge 1 commit intoTheSuperHackers:mainfrom
Conversation
GeneralsMD/Code/GameEngine/Source/GameLogic/System/GameLogic.cpp
Outdated
Show resolved
Hide resolved
My goal was to try match the vertical view as close as possible to retail, I think it's slightly less than compared to 4:3 but slightly more than 16:9 gentool + retail. I think it has a reasonable tradeoff considering the extended horizontal view. It also gives a reasonable view for aspect ratios between 4:3 and 1:1 as well. Giving a little more vertical view. Under 1:1 things are still quite broken but a lot needs changing in the view handling to properly support portrait mode. |
|
Yes GenTool did take some vertical view away to compensate for the wider view, perhaps in an attempt to keep conditions fair. Additional camera height does give a competitive advantage. Are we ok with giving advantages to Wide Screen? |
|
I wouldn't consider it an issue since most screens have been wide aspect For the past decade. If anything widescreen is the standard now. Ultrawide is where it becomes more of an issue. But we could look at implementing a locked aspect mode for the tactical view in future. Or another option could be implementing tactical zoom to let people zoom out to view the entire map etc. |
02a64e0 to
57fe78d
Compare
xezon
left a comment
There was a problem hiding this comment.
Simple implementation, but man the EA code is confusing.
GeneralsMD/Code/GameEngineDevice/Source/W3DDevice/GameClient/W3DView.cpp
Outdated
Show resolved
Hide resolved
GeneralsMD/Code/GameEngineDevice/Source/W3DDevice/GameClient/W3DView.cpp
Outdated
Show resolved
Hide resolved
GeneralsMD/Code/GameEngineDevice/Source/W3DDevice/GameClient/W3DView.cpp
Outdated
Show resolved
Hide resolved
GeneralsMD/Code/GameEngineDevice/Source/W3DDevice/GameClient/W3DView.cpp
Outdated
Show resolved
Hide resolved
GeneralsMD/Code/GameEngineDevice/Source/W3DDevice/GameClient/W3DView.cpp
Outdated
Show resolved
Hide resolved
57fe78d to
271ca5b
Compare
|
Tweaked the code so Still need to look at the zoom default handling. |
271ca5b to
f6c96d8
Compare
|
Updated with tweaked max and default zoom handling code. |
f6c96d8 to
c3c7b85
Compare
|
Small tweak to comments |
52f7de7 to
ab0b316
Compare
|
Reverted this to just have the camera height adjustments and nothing else. |
968ad71 to
e497ba8
Compare
|
Note for future: When raising up the camera, the following things need to be scaled as well:
|
e497ba8 to
a0b7db7
Compare
|
| Filename | Overview |
|---|---|
| Core/GameEngineDevice/Source/W3DDevice/GameClient/W3DView.cpp | Adds setCameraHeightAboveGroundLimitsToDefault (aspect-ratio-aware height scaling) and setZoomToMax (correct W3DView override); setZoomToDefault now only stops scripted cameras without setting zoom height — correct only when every call site also calls setZoomToMax(); redundant if/else in aspect ratio calculation |
| GeneralsMD/Code/GameEngine/Source/GameClient/InGameUI.cpp | Both init() and reset() replace setDefaultView with setCameraHeightAboveGroundLimitsToDefault() but omit the required setZoomToMax() call, leaving zoom/height uninitialised at those sites |
| Core/GameEngine/Source/GameClient/View.cpp | Adds setZoomToMax() base implementation that incorrectly adds m_maxHeightAboveGround to current height rather than setting it; the W3DView override is correct so game impact is limited to non-W3DView subclasses |
| GeneralsMD/Code/GameEngine/Source/GameLogic/ScriptEngine/ScriptActions.cpp | Replaces setDefaultView with separate calls in doCameraSetDefault; retains the heighScale typo; setPitch/setAngle calls carry the stopDoingScriptedCamera() side effect and do not update the default-pitch field |
| GeneralsMD/Code/GameEngine/Source/GameLogic/System/GameLogic.cpp | Adds setCameraHeightAboveGroundLimitsToDefault() before the existing reset sequence and appends setZoomToMax() after setZoomToDefault() — call ordering is correct |
| GeneralsMD/Code/GameEngine/Source/GameClient/GUI/GUICallbacks/Menus/OptionsMenu.cpp | Correctly calls both setCameraHeightAboveGroundLimitsToDefault() and setZoomToMax() after a resolution change on the shell map |
| Core/GameEngine/Include/GameClient/View.h | Adds setCameraHeightAboveGroundLimitsToDefault virtual and setZoomToMax virtual declarations; interface is clean |
| Core/GameEngineDevice/Include/W3DDevice/GameClient/W3DView.h | Declares setCameraHeightAboveGroundLimitsToDefault and setZoomToMax overrides; adds informational comment on setZoomToDefault about scripted-camera halt side effect |
| GeneralsMD/Code/GameEngine/Include/Common/GlobalData.h | Adds clarifying comment that m_maxCameraHeight/m_minCameraHeight represent the 4:3 base values; no functional changes |
| GeneralsMD/Code/GameEngine/Include/GameLogic/ScriptActions.h | Renames maxHeight parameter to heighScale (typo for heightScale) in doCameraSetDefault declaration |
Sequence Diagram
sequenceDiagram
participant GL as GameLogic::startNewGame
participant OM as OptionsMenu::saveOptions
participant UI as InGameUI::init / reset
participant TV as TheTacticalView (W3DView)
GL->>TV: setCameraHeightAboveGroundLimitsToDefault()
Note over TV: Computes aspectRatioScale,<br/>sets m_max/minHeightAboveGround
GL->>TV: setAngleToDefault()
GL->>TV: setPitchToDefault()
GL->>TV: setZoomToDefault()
Note over TV: stopDoingScriptedCamera()<br/>(does NOT set zoom height)
GL->>TV: setZoomToMax()
Note over TV: m_heightAboveGround = m_maxHeightAboveGround ✓
OM->>TV: setCameraHeightAboveGroundLimitsToDefault()
OM->>TV: setZoomToMax() ✓
UI->>TV: setCameraHeightAboveGroundLimitsToDefault()
Note over TV: ⚠️ setZoomToMax() NOT called<br/>zoom/height left uninitialised
Prompt To Fix All With AI
This is a comment left during a code review.
Path: Core/GameEngineDevice/Source/W3DDevice/GameClient/W3DView.cpp
Line: 2096-2103
Comment:
**Redundant if/else branches compute identical formula**
Both branches simplify to `fabs(1.0f + currentAspectRatio - baseAspectRatio)`. The conditional is unnecessary — the subtraction is commutative so splitting on whether `currentAspectRatio > baseAspectRatio` produces the same arithmetic expression in both arms.
```suggestion
aspectRatioScale = fabs(1.0f + currentAspectRatio - baseAspectRatio);
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (12): Last reviewed commit: "fix(view): Adjust default camera height ..." | Re-trigger Greptile
| /** doCameraSetDefault */ | ||
| //------------------------------------------------------------------------------------------------- | ||
| void ScriptActions::doCameraSetDefault(Real pitch, Real angle, Real maxHeight) | ||
| void ScriptActions::doCameraSetDefault(Real pitch, Real angle, Real heighScale) |
There was a problem hiding this comment.
syntax: typo: heighScale should be heightScale
| void ScriptActions::doCameraSetDefault(Real pitch, Real angle, Real heighScale) | |
| void ScriptActions::doCameraSetDefault(Real pitch, Real angle, Real heightScale) |
Prompt To Fix With AI
This is a comment left during a code review.
Path: GeneralsMD/Code/GameEngine/Source/GameLogic/ScriptEngine/ScriptActions.cpp
Line: 4611:4611
Comment:
**syntax:** typo: `heighScale` should be `heightScale`
```suggestion
void ScriptActions::doCameraSetDefault(Real pitch, Real angle, Real heightScale)
```
How can I resolve this? If you propose a fix, please make it concise.| void doCameraTetherNamed(const AsciiString& unit, Bool snapToUnit, Real play); | ||
| void doCameraStopTetherNamed(void); | ||
| void doCameraSetDefault(Real pitch, Real angle, Real maxHeight); | ||
| void doCameraSetDefault(Real pitch, Real angle, Real heighScale); |
There was a problem hiding this comment.
syntax: typo: heighScale should be heightScale
| void doCameraSetDefault(Real pitch, Real angle, Real heighScale); | |
| void doCameraSetDefault(Real pitch, Real angle, Real heightScale); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: GeneralsMD/Code/GameEngine/Include/GameLogic/ScriptActions.h
Line: 114:114
Comment:
**syntax:** typo: `heighScale` should be `heightScale`
```suggestion
void doCameraSetDefault(Real pitch, Real angle, Real heightScale);
```
How can I resolve this? If you propose a fix, please make it concise.a0b7db7 to
22e6c6b
Compare
22e6c6b to
6ea3918
Compare
6ea3918 to
09d7a25
Compare
| TheTacticalView->setHeight( TheDisplay->getHeight() ); | ||
| } | ||
| TheTacticalView->setDefaultView(0.0f, 0.0f, 1.0f); | ||
| TheTacticalView->setCameraHeightAboveGroundLimitsToDefault(); |
There was a problem hiding this comment.
Missing setZoomToMax() call after setCameraHeightAboveGroundLimitsToDefault(). Similar to GameLogic::startNewGame() and OptionsMenu::saveOptions(), this should call setZoomToMax() to actually set the camera height.
| TheTacticalView->setCameraHeightAboveGroundLimitsToDefault(); | |
| TheTacticalView->setCameraHeightAboveGroundLimitsToDefault(); | |
| TheTacticalView->setZoomToMax(); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: GeneralsMD/Code/GameEngine/Source/GameClient/InGameUI.cpp
Line: 1380:1380
Comment:
Missing `setZoomToMax()` call after `setCameraHeightAboveGroundLimitsToDefault()`. Similar to `GameLogic::startNewGame()` and `OptionsMenu::saveOptions()`, this should call `setZoomToMax()` to actually set the camera height.
```suggestion
TheTacticalView->setCameraHeightAboveGroundLimitsToDefault();
TheTacticalView->setZoomToMax();
```
How can I resolve this? If you propose a fix, please make it concise.| TheControlBar->reset(); | ||
|
|
||
| TheTacticalView->setDefaultView(0.0f, 0.0f, 1.0f); | ||
| TheTacticalView->setCameraHeightAboveGroundLimitsToDefault(); |
There was a problem hiding this comment.
Missing setZoomToMax() call after setCameraHeightAboveGroundLimitsToDefault(). Without this, the camera zoom won't be properly initialized during reset.
| TheTacticalView->setCameraHeightAboveGroundLimitsToDefault(); | |
| TheTacticalView->setCameraHeightAboveGroundLimitsToDefault(); | |
| TheTacticalView->setZoomToMax(); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: GeneralsMD/Code/GameEngine/Source/GameClient/InGameUI.cpp
Line: 2150:2150
Comment:
Missing `setZoomToMax()` call after `setCameraHeightAboveGroundLimitsToDefault()`. Without this, the camera zoom won't be properly initialized during reset.
```suggestion
TheTacticalView->setCameraHeightAboveGroundLimitsToDefault();
TheTacticalView->setZoomToMax();
```
How can I resolve this? If you propose a fix, please make it concise.b07ab06 to
499d6ef
Compare
499d6ef to
27aa725
Compare
| TheTacticalView->setPitch(pitch); | ||
| TheTacticalView->setAngle(angle); |
There was a problem hiding this comment.
setPitch/setAngle unexpectedly stop scripted cameras
Both W3DView::setPitch and W3DView::setAngle call stopDoingScriptedCamera() as a side effect. The original setDefaultView only updated m_defaultPitch and m_maxHeightAboveGround without touching scripted camera state at all.
This means that any call to doCameraSetDefault from a script while a scripted camera animation is running will silently terminate that animation — a behavioural change not present before this PR. The function name implies updating defaults, not interrupting live camera paths.
Additionally, the old code stored the pitch in m_defaultPitch, so a later setPitchToDefault() would correctly restore it. The new setPitch() call only writes to m_pitch (the current pitch), leaving m_defaultPitch unchanged. If the engine resets to default pitch elsewhere, the value set by this script action will be lost.
Consider using a method that updates the defaults without calling stopDoingScriptedCamera(), or explicitly documenting the intended behavioural change if stopping the scripted camera is desired.
Prompt To Fix With AI
This is a comment left during a code review.
Path: GeneralsMD/Code/GameEngine/Source/GameLogic/ScriptEngine/ScriptActions.cpp
Line: 4607-4608
Comment:
**`setPitch`/`setAngle` unexpectedly stop scripted cameras**
Both `W3DView::setPitch` and `W3DView::setAngle` call `stopDoingScriptedCamera()` as a side effect. The original `setDefaultView` only updated `m_defaultPitch` and `m_maxHeightAboveGround` without touching scripted camera state at all.
This means that any call to `doCameraSetDefault` from a script while a scripted camera animation is running will silently terminate that animation — a behavioural change not present before this PR. The function name implies updating defaults, not interrupting live camera paths.
Additionally, the old code stored the pitch in `m_defaultPitch`, so a later `setPitchToDefault()` would correctly restore it. The new `setPitch()` call only writes to `m_pitch` (the *current* pitch), leaving `m_defaultPitch` unchanged. If the engine resets to default pitch elsewhere, the value set by this script action will be lost.
Consider using a method that updates the defaults without calling `stopDoingScriptedCamera()`, or explicitly documenting the intended behavioural change if stopping the scripted camera is desired.
How can I resolve this? If you propose a fix, please make it concise.|
This is only updating at times since i use this PR for the Legi build |
27aa725 to
f0dc49b
Compare
f0dc49b to
f5167b0
Compare
|
How far are we from having this in the main build? I want to try all the new things... |
5b9c231 to
5617740
Compare
5617740 to
a076193
Compare


Merge with Rebase
This PR adjusts the default max camera height to compensate for different screen aspect ratios.
Previous work fixed the vertical field of view to match retail and adjusted the horizontal fields of view.
But this resulted in significant distortion being observed in the periphery of the view and lower corners.
This distortion was also observed at the 16:9 aspect ratio and became significantly worse as the aspect ratio increased.
By maintaining the original 60 degrees horizontal field of view, while allowing the game to narrow the vertical field of view, increasing the camera height results in minimal to no distortion being observed.
This PR does not fix any other known camera issues and focuses on giving a playable default view.TODO