Skip to content

bugfix(gui): implement resolution scaling for unit health and info#1607

Open
Mauller wants to merge 2 commits into
TheSuperHackers:mainfrom
Mauller:fix-unit-info-res-scaling
Open

bugfix(gui): implement resolution scaling for unit health and info#1607
Mauller wants to merge 2 commits into
TheSuperHackers:mainfrom
Mauller:fix-unit-info-res-scaling

Conversation

@Mauller
Copy link
Copy Markdown

@Mauller Mauller commented Sep 19, 2025

This PR implements raw resolution and aspect ratio scaling for the unit health bar and other info icons.
This PR also adds functions to the display for calculating the display width and height relative to the default resolutions. And a function to calculate the diference in the current aspect ratio from the default.

Zoom based scaling to keep unit info scaled relative to units will be looked at in another PR.

There is also a small fix for the veterancy icon positioning as this was not implemented properly in the original code and broke when the health bar is scaled properly.

The only thing not scaled in this at the moment is the bombed icons, these will be dealt with in another PR.
This is due to icon placement changes being required so the icon is placed on the target instead of at the feet of the unit placing the bomb.

Comment thread GeneralsMD/Code/GameEngine/Source/GameClient/Drawable.cpp Outdated

// do this so health bar doesn't get too skinny or fat after scaling
//healthBoxHeight = max(3.0f, healthBoxHeight);
healthBoxHeight = 3.0f;
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The retrieved health box height from the object is already 3.0f, at higher resoutions this code prevents the health box from scaling relative to the vehicles and looks wrong. Which is why i removed it.

I reinstated the clamp on the max size of the health bar to prevent it shrinking too much when the resolution is scaled. This will also come into play more when zoom scaling is added.

@Mauller Mauller self-assigned this Sep 19, 2025
@Mauller Mauller added Bug Something is not working right, typically is user facing GUI For graphical user interface Minor Severity: Minor < Major < Critical < Blocker Gen Relates to Generals ZH Relates to Zero Hour Major Severity: Minor < Major < Critical < Blocker and removed Minor Severity: Minor < Major < Critical < Blocker labels Sep 19, 2025
@xezon
Copy link
Copy Markdown

xezon commented Sep 20, 2025

Is this a piece split off of #1573 ?

@Mauller
Copy link
Copy Markdown
Author

Mauller commented Sep 20, 2025

Is this a piece split off of #1573 ?

Yes, this only implements the resolution scaling of the UI elements and some small fixes to the veterancy icon and compute health region code.

I did not close the original as I will build the zoom scaling off this pr when finalised and merged.

Comment thread GeneralsMD/Code/GameEngine/Source/GameClient/Drawable.cpp Outdated
Comment thread GeneralsMD/Code/GameEngine/Include/GameClient/Display.h Outdated
Comment thread GeneralsMD/Code/GameEngine/Include/GameClient/Display.h Outdated
@@ -2959,8 +2954,8 @@ void Drawable::drawContained( const IRegion2D *healthBarRegion )
#else
Real scale = 1.0f;
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like EA already tried to do some scaling here with TheGlobalData->m_ammoPipScaleFactor in SCALE_ICONS_WITH_ZOOM_ML. Does SCALE_ICONS_WITH_ZOOM_ML still have any relevance? What does it do? If it is not useful, can that be removed before we add a new scaling for it?

Copy link
Copy Markdown
Author

@Mauller Mauller Sep 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Scale icons with zoom does not have any real relevance once I properly implement the zoom scaling.

But the idea was to rescale the info icons as you zoomed into units so their size was kept uniform relative to the health bar. Which is what I reimplemented.

We could still make use of the pipscale factor though.

Comment thread GeneralsMD/Code/GameEngine/Source/GameClient/Drawable.cpp Outdated
Comment thread GeneralsMD/Code/GameEngine/Source/GameClient/Drawable.cpp Outdated
@Mauller Mauller force-pushed the fix-unit-info-res-scaling branch from 20904b1 to 09d6dae Compare September 20, 2025 16:17
@Mauller
Copy link
Copy Markdown
Author

Mauller commented Sep 20, 2025

Quick Rebase

@Mauller Mauller force-pushed the fix-unit-info-res-scaling branch from 09d6dae to d068267 Compare September 20, 2025 17:11
@Mauller
Copy link
Copy Markdown
Author

Mauller commented Sep 20, 2025

Tweaked this now so the followup can implement user based options within InGameUI.

This version also reduces the overhead a little since the scaling factor is only calculated once at initialisation and when the resolution gets changed.

So with this, the unit info acts the same as with retail, but scales all the unit gui elements properly with resolution.

Comment thread GeneralsMD/Code/GameEngine/Source/GameClient/Drawable.cpp
Comment thread GeneralsMD/Code/GameEngine/Source/GameClient/InGameUI.cpp Outdated
@Mauller Mauller force-pushed the fix-unit-info-res-scaling branch from d068267 to a64a7ee Compare September 21, 2025 09:45
@Mauller
Copy link
Copy Markdown
Author

Mauller commented Sep 21, 2025

Moved the scale variable into the SCALE_ICONS_WITH_ZOOM_ML for the draw veterancy function

@Mauller Mauller force-pushed the fix-unit-info-res-scaling branch from a64a7ee to c172cfe Compare September 21, 2025 10:00
@Mauller
Copy link
Copy Markdown
Author

Mauller commented Sep 21, 2025

Tweaked veterancy icon position, it should be in exactly the same place now, it might look slightly offset but that's actually the health bar being marginally shifted left due to the above mentioned change.

@xezon
Copy link
Copy Markdown

xezon commented Sep 21, 2025

Health bar size is not correct on higher solution.

In 800x600, health bar is 1 pixel high. In 1600x1200 it is 4 pixels high.

healthbar

@Mauller
Copy link
Copy Markdown
Author

Mauller commented Sep 21, 2025

Health bar size is not correct on higher solution.

In 800x600, health bar is 1 pixel high. In 1600x1200 it is 4 pixels high.

healthbar

Yes because it's scaling with the resolution, it's proportions are the same compared to 800x600

If the health bar remaind the same thickness as it is at 800x600 then you will barely see anything of it at higher resolutions.

@xezon
Copy link
Copy Markdown

xezon commented Sep 21, 2025

I think it is wrong. If health bar is 1px at 800x600, then it should be 2px at 1600x1200.

@Mauller
Copy link
Copy Markdown
Author

Mauller commented Sep 21, 2025

I think it is wrong. If health bar is 1px at 800x600, then it should be 2px at 1600x1200.

Its not wrong, it's because of the way the health bar is setup. The border does not scale with the health bar and remains at only 1 pixel in height.

The health bar region is 6 pixels high, which is double the default of 3 at 800x600, but there is always a 1 pixel border.

This means that at 800x600 there is only a single pixel for the health bar info itself while theres 4 at 1600x1200

So overall the health bar is scaled properly, it's just that relatively more of the health bar is taken up by the border at 800x600

@Mauller
Copy link
Copy Markdown
Author

Mauller commented Sep 21, 2025

The other way to work this would be to also scale the health bar border at higher resolutions, it would also make the health bar more visible when health is getting depleted and should keep the health info itself scaled similar to 800x600

Comment thread GeneralsMD/Code/GameEngine/Source/GameClient/InGameUI.cpp Outdated
Comment thread GeneralsMD/Code/GameEngine/Source/GameClient/InGameUI.cpp Outdated
@xezon
Copy link
Copy Markdown

xezon commented Sep 21, 2025

I think it is wrong. If health bar is 1px at 800x600, then it should be 2px at 1600x1200.

Its not wrong, it's because of the way the health bar is setup. The border does not scale with the health bar and remains at only 1 pixel in height.

The health bar region is 6 pixels high, which is double the default of 3 at 800x600, but there is always a 1 pixel border.

This means that at 800x600 there is only a single pixel for the health bar info itself while theres 4 at 1600x1200

So overall the health bar is scaled properly, it's just that relatively more of the health bar is taken up by the border at 800x600

I think visually it should look twice as big. Right now it looks 4 times as big.

@Mauller
Copy link
Copy Markdown
Author

Mauller commented Sep 21, 2025

I think it is wrong. If health bar is 1px at 800x600, then it should be 2px at 1600x1200.

Its not wrong, it's because of the way the health bar is setup. The border does not scale with the health bar and remains at only 1 pixel in height.
The health bar region is 6 pixels high, which is double the default of 3 at 800x600, but there is always a 1 pixel border.
This means that at 800x600 there is only a single pixel for the health bar info itself while theres 4 at 1600x1200
So overall the health bar is scaled properly, it's just that relatively more of the health bar is taken up by the border at 800x600

I think visually it should look twice as big. Right now it looks 4 times as big.

I am working on something that should do that, some of the functions aren't working quite right for drawing rectangles with outlines so just currently investigating that.

@Mauller
Copy link
Copy Markdown
Author

Mauller commented Sep 21, 2025

Well that was a pain to dig through and find the bottom of the border drawing issue, but this should literally be looking good now.

Copy link
Copy Markdown

@xezon xezon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks promising, but there are still some things.

Comment thread GeneralsMD/Code/Libraries/Source/WWVegas/WW3D2/render2d.cpp Outdated
Comment thread GeneralsMD/Code/GameEngine/Source/GameClient/Display.cpp Outdated
Comment thread GeneralsMD/Code/GameEngine/Source/GameClient/InGameUI.cpp Outdated
Comment thread GeneralsMD/Code/Libraries/Source/WWVegas/WW3D2/render2d.cpp Outdated
@@ -3925,23 +3916,23 @@ void Drawable::drawHealthBar(const IRegion2D* healthBarRegion)

}

Real healthBoxWidth = healthBarRegion->width();
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Health bar width looks a bit different, noticeable on Troopcrawler. See right side.

image

Copy link
Copy Markdown
Author

@Mauller Mauller Sep 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IT looks like it comes up to the black viewport to me in both, the thing that makes it look significnatly bigger is the placement of the contained pips.

Something i noticed is that the contained pips seem to integer scale and have a fixed seperation.#
So they will pop sideways at times in different resolutions or zoom levels.
When zooming with zoom scaling enabled they would often pop and move about by nearly an entire pip to the left or right

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At 800x600
image

at 1600x1200
image

i think like with the health bar border, i might need to also scale the pip spacing

Copy link
Copy Markdown
Author

@Mauller Mauller Sep 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes scaling the pip spacing made it match
1600x1200
image

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will make sure to do the same for ammo pips

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In 1280x720 the discrepancy still occurs. Maybe is inevitable because of integer rounding?

Copy link
Copy Markdown
Author

@Mauller Mauller Sep 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The images themselves are integer rounded within the image draw functions. so you then have that compounding loss over every extra pip while there is only a single rounding on the health bars width.

@@ -3925,23 +3916,23 @@ void Drawable::drawHealthBar(const IRegion2D* healthBarRegion)

}

Real healthBoxWidth = healthBarRegion->width();
Real healthBoxHeight = max(3, healthBarRegion->height());
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With 1280x720 the scaling of the health bar will be fractionized. This creates an unfortunate coloring with washed greens in the center.

Can the positioning or rounding be optimized in a way to preserve finer details on fractional scale, for example by moving the pixels by 0.5 in both x and y ?

1280x720 at 300% zoom

1280x720

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay to correct this i have to floor the scaled thickness of the border, this then makes sure that the border does not grow too large in intermediate resolutions.

800x600
image

1280x720
image

1600x1200
image

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are the lines integer? If we could have float coords for the bars then the intermediate center green bar for example could cover 1 full pixel line + 2x 0.5 pixel line above and below. This should then scale nicely from 800x600 towards 1600x1200 and beyond.

Technically a half pixel line means a line with 50% opacity.

Copy link
Copy Markdown

@xezon xezon Sep 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested 1280x720 and the health looks too big (the center green part). I suggest explore fractionalized drawing.

Copy link
Copy Markdown
Author

@Mauller Mauller Sep 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could integer scale the height of the health bar, this way it always looks clear and sharp.

Then the health bar region is only ever multiples of 3 high then the green bit scales as 1, 2, 3 pixels high etc.

Keeping the sharp look makes it clearer to see, i think having any kind of blending will make it less distinct, considering we are dealing with something only a few pixels in size.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tweaked it so the border part is less transparent and decreases with the health

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does not scale correctly:

healthbars1400

I think the health bar scaling will be easier to test in combination with the zoom scaling. So for testing, perhaps enable the zoom scaling. I think it is important to have fractional scale, especially on the health bars, not just for resolution, but also for zoom. We do not want blocky transitions, but smooth transitions. Try to get rid of the integer positions and only lock the pixels on the screen pixel grid, not on the health bar itself.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The health bar itself is already scaled in pixels, the underlying functions that draw to the screen deal in screen coordinates of pixels. This includes the line and rect drawing functions.

I can enable the zoom scaling for the vertical axis of the health bar if you want to look more at that.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the bar needs to be scaled in fractions of pixels, and positioned in pixels.

Do you need help with the implementation for it?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I should be alright for the minute, need to experiment with getting rect and line functions working that don't use whole pixel coordinates.

@xezon xezon added Enhancement Is new feature or request and removed Bug Something is not working right, typically is user facing labels Sep 23, 2025
@Mauller Mauller force-pushed the fix-unit-info-res-scaling branch from 6ca0750 to bdca8c3 Compare September 23, 2025 19:23
@Mauller
Copy link
Copy Markdown
Author

Mauller commented Sep 23, 2025

Should be looking better now, ho ho ho.

@Mauller Mauller force-pushed the fix-unit-info-res-scaling branch 3 times, most recently from 4b5bfae to 542f98b Compare September 26, 2025 17:17
@Mauller Mauller force-pushed the fix-unit-info-res-scaling branch from 542f98b to f7f2ee4 Compare November 19, 2025 18:06
@Mauller Mauller force-pushed the fix-unit-info-res-scaling branch from d75242a to 33cdc71 Compare November 28, 2025 17:17
@Mauller Mauller force-pushed the fix-unit-info-res-scaling branch from 33cdc71 to 58d6451 Compare December 12, 2025 14:36
@Mauller
Copy link
Copy Markdown
Author

Mauller commented Dec 12, 2025

Quick rebase to catch up with main

@Mauller Mauller force-pushed the fix-unit-info-res-scaling branch from 58d6451 to 335f413 Compare December 12, 2025 17:22
@Mauller
Copy link
Copy Markdown
Author

Mauller commented Dec 12, 2025

Removed the changes from the extra commit and integer scale the health bar height now based on screen height scale.

@xezon
Copy link
Copy Markdown

xezon commented Dec 13, 2025

Is this ready for testing?


}

Real healthBoxWidth = healthBarRegion->width();
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The health bar is significantly wider than originally - at 16:9 aspect. Maybe more than twice as wide.

In principle widening is ok, because the original bar indeed looks narrow, but it looks too much in comparison. I expect this will cause complaints from long time players. I see this is mainly as pronounced in Wide Screen, so it looks like something was fixed that disproportionally affects Wide Screen Resolution. Perhaps make the default width closer to the original at 4:3 aspect (currently is a bit wider at 4:3) and then provide a User Option to scale the width, from 0 to N. This way user can fine the width to his preference, which likely will be required to satisfy taste.

Also, the fixed health bar shows a proper outline on damage, whereas the original was just one color. But this means visibility on the right side is a bit reduced. Maybe fill the right side of the health bar side with less transparency to somewhat preserve the original look more, but still show the outline. You can also play with the opacity of the outline to fine tune and compensate for visibility changes.

Left side is Original 1920x1080
Right side in Patched 1920x1080

Compared at about the same height.

image

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The health bar is wider because it is scaled to how it looks compared to 800x600

In the original health bar, the drawOpenRect function was broken meaning it was not properly drawing a rectangular border when it should have been. This is shown by the weird extra pixels under the bottom of the health bar and it not being perfectly rectangular.

The fixed outline makes it easier to see the amount of damage taken and gives it a much cleaner look.

Copy link
Copy Markdown

@xezon xezon Dec 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it is fixed, which is good, but it is also now much different than the original in 16:9. And in 4:3 it is a bit wider than it used to be. This means we need to do something to give users the option to return to the original look, if they prefer it that way.

I say this, because this change would upset me to some degree and so I expect it can also cause upset for other players. And I am not even part of the super conservative group of people.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you compare 800x600 and 1600x1200 the width of the health bar is the same as retail compared to retail 800x600.

It is slightly shifted to the right due to fixes in the drawing of the health bar.

For scaling the UI elements, we were going to add the ability to change those in a followup PR, this PR was just to get things to scale up properly at higher resolutions in relation to 800x600 retail.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you force scale it to retail now and unlock rescale with option in follow up change?

Copy link
Copy Markdown
Author

@Mauller Mauller Jan 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets keep the scaling relative to what it was meant to look like at 800x600 in this change as originally discussed.
It would make the code awkward when trying to scale it to 1080p in this change.
I can add the user option to rescale things in one of the folloup PR's, that way users can adjust it and we can adjust the default scale value.

I still need to add zoom scaling and fix the bombed icon stuff after this PR as well as add the user scaling options.


Real InGameUI::getUnitHealthbarScaleFactor()
{
return m_healthResolutionScaleFactor;
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These scale factors are the preliminary work for adding user based scaling options, but i might tweak them a bit further in a followup PR when implementing that.

@Mauller Mauller force-pushed the fix-unit-info-res-scaling branch from 335f413 to a482938 Compare April 14, 2026 21:02
@Mauller
Copy link
Copy Markdown
Author

Mauller commented Apr 14, 2026

Just updated after recent zoom handling changes, had to tweak the zoom scaling factor to replicate the original health bar width values.

This is ready to go otherwise.

@greptile-apps
Copy link
Copy Markdown

greptile-apps Bot commented Apr 14, 2026

Greptile Summary

This PR wires up resolution-aware scaling for unit health bars and info icons by adding getWidthScale()/getHeightScale() to Display, a calcUnitInfoScaleFactor() recalculation hook on resolution change, and propagating the scale factor through every icon-drawing path in Drawable.cpp. It also fixes the veterancy icon offset to use the rendered health-bar width and corrects corner rendering in Render2DClass::Add_Outline to properly overlap at any line width.

  • P1: drawVeterancy dereferences healthBarRegion at line 3768 without a null guard — all parallel draw methods protect against this, and a one-line fix suffices.

Confidence Score: 4/5

Safe to merge after adding a null guard in drawVeterancy; all other findings are style/cleanup.

One P1 latent null-pointer dereference in drawVeterancy remains. While current guards coincidentally prevent a crash, the missing null check is a real defect that mirrors the explicit guard present in every other draw method in the same file. The remaining findings are P2 style issues (public member variables, redundant field/accessor, minor typo).

GeneralsMD/Code/GameEngine/Source/GameClient/Drawable.cpp (drawVeterancy null guard), GeneralsMD/Code/GameEngine/Include/GameClient/InGameUI.h (public member placement)

Important Files Changed

Filename Overview
GeneralsMD/Code/GameEngine/Source/GameClient/Drawable.cpp Core scaling changes for health bar and all icon renders; contains a P1 null pointer dereference in drawVeterancy and a minor typo in a comment tag.
GeneralsMD/Code/GameEngine/Include/GameClient/InGameUI.h Adds two scaling fields and three methods; both member variables are incorrectly placed in the public section instead of private.
GeneralsMD/Code/GameEngine/Source/GameClient/InGameUI.cpp Implements calcUnitInfoScaleFactor, getUnitInfoScaleFactor, and getUnitHealthbarScaleFactor; the health-specific field and accessor are redundant/unused.
GeneralsMD/Code/GameEngine/Include/GameClient/Display.h Declares getWidthScale() and getHeightScale() in the Display scaling methods block; clean addition.
GeneralsMD/Code/GameEngine/Source/GameClient/Display.cpp Implements getWidthScale and getHeightScale using existing DEFAULT_DISPLAY_WIDTH/HEIGHT constants; straightforward and correct.
GeneralsMD/Code/GameEngine/Include/GameLogic/Object.h Introduces the defaultHealthBoxHeight = 3.0f global const to replace scattered magic numbers.
GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Object.cpp Replaces magic 3.0f literals with defaultHealthBoxHeight; the pre-existing min/max inversion (min(3,max(5,...))) is unchanged and not introduced by this PR.
GeneralsMD/Code/Libraries/Source/WWVegas/WW3D2/render2d.cpp Fixes Add_Outline to use lineWidthOffset (width/2) so lines correctly overlap at corners and scale properly with any line width.
GeneralsMD/Code/GameEngine/Source/GameClient/GUI/GUICallbacks/Menus/OptionsMenu.cpp Adds calcUnitInfoScaleFactor() call after resolution change in Options menu; correct hook-up.
GeneralsMD/Code/GameEngine/Source/GameClient/GUI/GUICallbacks/Menus/MainMenu.cpp Adds calcUnitInfoScaleFactor() call in DeclineResolution; correct hook-up mirroring the OptionsMenu change.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    RC[Resolution Change\nOptionsMenu / MainMenu] --> CS[InGameUI::calcUnitInfoScaleFactor]
    CS --> WS[TheDisplay::getWidthScale]
    CS --> HS[TheDisplay::getHeightScale]
    WS --> SF[m_unitInfoResolutionScaleFactor = max\nwidth scale, height scale]
    HS --> SF

    DI[Drawable::drawIconUI] --> CR[computeHealthRegion]
    CR --> HBR[IRegion2D* healthBarRegion]
    HBR -->|non-null| DHB[drawHealthBar\noutline size scales with height]
    HBR -->|non-null| DVT[drawVeterancy\n⚠ no null guard]
    HBR -->|non-null| DICO[drawEmoticon / drawAmmo\ndrawContained / drawHealing\ndrawEnthusiastic / drawDisabled\ndrawBattlePlans]
    CR -->|worldToScreen fails| NULL[healthBarRegion = nullptr]
    NULL -->|passed to drawVeterancy| CRASH[⚠ potential null deref\nat healthBarRegion->width]

    SF -->|getUnitInfoScaleFactor| DVT
    SF -->|getUnitInfoScaleFactor| DICO
    HS -->|getHeightScale direct| CR
Loading
Prompt To Fix All With AI
This is a comment left during a code review.
Path: GeneralsMD/Code/GameEngine/Source/GameClient/Drawable.cpp
Line: 3768

Comment:
**Null pointer dereference on `healthBarRegion`**

`healthBarRegion` is `nullptr` whenever `computeHealthRegion` returns `FALSE` (e.g., the unit is off-screen / `worldToScreen` fails). All other draw methods guard against this (`drawHealthBar` has an early `if (!healthBarRegion) return;`), but `drawVeterancy` dereferences it unconditionally. While the function's own `worldToScreen` call happens to provide a coincidental guard in practice, this relies on an implicit assumption that is fragile and could crash if the failure paths ever diverge.

```suggestion
	screenCenter.x += (healthBarRegion ? healthBarRegion->width() : 0) * 0.65f;
```

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: GeneralsMD/Code/GameEngine/Include/GameClient/InGameUI.h
Line: 588-594

Comment:
**Member variables placed in `public:` section**

`m_unitInfoResolutionScaleFactor` and `m_healthResolutionScaleFactor` are declared in the `public:` section, exposing internal state to callers. They should be moved to the `private:` section below, alongside the `getUnitInfoScaleFactor()` / `getUnitHealthbarScaleFactor()` accessors that already provide the controlled interface.

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: GeneralsMD/Code/GameEngine/Source/GameClient/InGameUI.cpp
Line: 6384-6398

Comment:
**`m_healthResolutionScaleFactor` and `getUnitHealthbarScaleFactor()` are redundant**

Both member variables are always assigned the same value in `calcUnitInfoScaleFactor()`, and `getUnitHealthbarScaleFactor()` is never called anywhere in the codebase (health bar height scaling in `computeHealthRegion` uses `TheDisplay->getHeightScale()` directly). Consider removing the duplicate field and dead accessor to avoid drift if the two values are ever meant to differ.

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: GeneralsMD/Code/GameEngine/Source/GameClient/Drawable.cpp
Line: 2687

Comment:
**Typo in attribution tag**

"TheSuperHacker" is missing the final `s`.

```suggestion
	// TheSuperHackers @info Original zoom at default height had a value of ~1.35
```

How can I resolve this? If you propose a fix, please make it concise.

Reviews (1): Last reviewed commit: "fix(gui): implement resolution scaling f..." | Re-trigger Greptile

return;

screenCenter.x += healthBoxWidth * scale * 0.5f;
screenCenter.x += healthBarRegion->width() * 0.65f;
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Null pointer dereference on healthBarRegion

healthBarRegion is nullptr whenever computeHealthRegion returns FALSE (e.g., the unit is off-screen / worldToScreen fails). All other draw methods guard against this (drawHealthBar has an early if (!healthBarRegion) return;), but drawVeterancy dereferences it unconditionally. While the function's own worldToScreen call happens to provide a coincidental guard in practice, this relies on an implicit assumption that is fragile and could crash if the failure paths ever diverge.

Suggested change
screenCenter.x += healthBarRegion->width() * 0.65f;
screenCenter.x += (healthBarRegion ? healthBarRegion->width() : 0) * 0.65f;
Prompt To Fix With AI
This is a comment left during a code review.
Path: GeneralsMD/Code/GameEngine/Source/GameClient/Drawable.cpp
Line: 3768

Comment:
**Null pointer dereference on `healthBarRegion`**

`healthBarRegion` is `nullptr` whenever `computeHealthRegion` returns `FALSE` (e.g., the unit is off-screen / `worldToScreen` fails). All other draw methods guard against this (`drawHealthBar` has an early `if (!healthBarRegion) return;`), but `drawVeterancy` dereferences it unconditionally. While the function's own `worldToScreen` call happens to provide a coincidental guard in practice, this relies on an implicit assumption that is fragile and could crash if the failure paths ever diverge.

```suggestion
	screenCenter.x += (healthBarRegion ? healthBarRegion->width() : 0) * 0.65f;
```

How can I resolve this? If you propose a fix, please make it concise.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Enhancement Is new feature or request Gen Relates to Generals GUI For graphical user interface Major Severity: Minor < Major < Critical < Blocker ZH Relates to Zero Hour

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants