Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introducing backing scale #1269

Merged
merged 27 commits into from
Jun 26, 2023
Merged

Conversation

colincornaby
Copy link
Contributor

@colincornaby colincornaby commented Dec 5, 2022

This change introduces the idea of a backing scale into the engine. Backing scale refers to the difference between the resolution of the window that Uru is running in, and the resolution of the frame buffer backing it.

For example, on a 4k Retina display, the window may only have a resolution of 1920x1080, while the actual Metal/DX/OpenGL frame buffer has a resolution of 3840x2160. The backing scale would be 2.

Different UI elements can use the backing scale to resize accordingly. This change alters the behavior of progress bars and the intro movie to account for the backing scale. Cursor should also change with the backing scale - I'm still working on pulling that together for this pull request.

Text rendering should also take into account the backing scale (although this PR does not make all the required changes.) A backing scale of 2 would imply that a 12 point font should render at twice the DPI. I've made some initial progress for this on macOS.

Backing scale is also useful if Uru ever chose to use DLSS/MetalFX upscaling. If the engine is under-rendering, backing scale can be used to push the UI elements back into a size the user expects.

@colincornaby
Copy link
Contributor Author

I'm looking at cursor scale again. Cursor scale is its own function right now in the Mac client. In some ways this makes sense - plMouseDevice is not owned by plClient and can't trivially just inherit settings. There are accessibility reasons that the cursor may also want to be its own scale down the road.

I may reintroduce the cursor scale as is - even though it's parallel to the backing scale and not directly affected by it. There could be further iteration on cursor scale later.

@colincornaby
Copy link
Contributor Author

Something weird is going on in Windows font rendering. It's not behaving how I expect it to, and this pull request is causing font spacing to not behave correctly on Windows. Still checking it over.

@colincornaby
Copy link
Contributor Author

I'm kind of fumbling around in the Win32 client (never done serious Win32 before) and I've got backing and cursor scale hooked up in Windows as well.

I enabled HiDPI in Windows by tweaking the manifest. I have not committed that change in since it probably requires more testing, and there have been known issues around HiDPI.

<?xml version="1.0" encoding="UTF-8"?>
<assembly xmlns="urn:schemas-microsoft-com:asm.v1" manifestVersion="1.0" xmlns:asmv3="urn:schemas-microsoft-com:asm.v3">
	<asmv3:application>
		<asmv3:windowsSettings>
			<dpiAware xmlns="http://schemas.microsoft.com/SMI/2005/WindowsSettings">true</dpiAware>
		</asmv3:windowsSettings>
	</asmv3:application>
  <dependency>
    <dependentAssembly>
      <assemblyIdentity type="win32" name="Microsoft.Windows.Common-Controls" version="6.0.0.0" processorArchitecture="*" publicKeyToken="6595b64144ccf1df" language="*"/>
    </dependentAssembly>
  </dependency>
</assembly>

@Hoikas
Copy link
Member

Hoikas commented Dec 10, 2022

I haven't read the code yet, but we probably want to avoid using the manifest for this because DPI Awareness v2 in the manifest will lock us to Windows 10 v1703. Instead, we'll want to use LoadLibrary to call the appropriate SetThreadDpiAwareness function.

@colincornaby
Copy link
Contributor Author

Thanks! I was unsure how to opt in to functions from newer versions of Windows. There are better DPI check functions to use on newer Windows as well.

I'm also unsure if cursor scale and backing scale should be more closely related or stay separate options - but that's why this PR is a draft.

@Hoikas
Copy link
Member

Hoikas commented Jan 8, 2023

I've built a set of DPI awareness changes for Windows on top of this PR on my fork that might be useful. My branch seems to fix the problems in #636.

@colincornaby
Copy link
Contributor Author

colincornaby commented Feb 17, 2023

Your changes look good to me.

I'll reset this to your branch's head and then graduate this to a full PR. Your comments indicate there might be a few things not done yet, but we can look at that on deeper review.

@colincornaby
Copy link
Contributor Author

I wrote that I'd bring in your branch's head. It's already here! Never mind.

@colincornaby colincornaby marked this pull request as ready for review February 17, 2023 06:36
@colincornaby
Copy link
Contributor Author

@Hoikas - I'm going to look at this again on macOS. Was there any more work you wanted to do on Windows? So far this has worked well on Windows for me, but I know there are some todos in the code comments.

@Hoikas
Copy link
Member

Hoikas commented Mar 19, 2023

I don't think there's anything else I want to do right now.

@colincornaby
Copy link
Contributor Author

I'll do a final checkover. I'm fine dealing with any Mac specific issues directly on the Mac branch. But I do want to make sure there isn't anything critical with the core implementation.

@colincornaby
Copy link
Contributor Author

Something is subtlety wrong with Windows font rendering at HiDPI. The spacing is off. I think it's due to an issue with the font map that's created. I'll investigate more.

@colincornaby
Copy link
Contributor Author

colincornaby commented Mar 20, 2023

Some details about the Windows font issue I'm looking at.

Here's what the current master branch looks like on the load screen:
1x Loading

And here's what this branch looks like on a HiDPI display:
2x Loading

The HiDPI issue has some subtle issues. The "Downloading" text is slightly too high and the detail text below the progress bar is spaced slightly too close.

Here's a graphic that better shows the issue:
ComparisonSideBySide

I spent some time going through the rendering code, and have started to believe the issue might be in the Win32 code that creates the font texture. On the left is the HiDPI font texture, and on the right is the normal font texture. I did a 2x nearest neighbor scale on the low rest font texture to make it comparable to the 1x texture.

Comparison

There seems to have been a padding change over the original 1x map. This could be working its way into the renderer causing the text to be pushed up. That would push the top text away from the progress car, and the bottom text closer to the progress bar.

This is my best lead so far. Examining the original Direct3D rendering with PIX and the metrics being created on the fly - everything in those components seems to be in order. The font map is the only thing I'm finding that looks wrong.

My strength is in Quartz2D text rendering but I'll do some Win32 digging to see what I can figure out. I might pull this code out into it's own app to do some quicker debugging.

@Hoikas
Copy link
Member

Hoikas commented Mar 20, 2023

It's probable that there are some non-DPI aware APIs being called in the debug text renderer that I didn't catch. 😞

@colincornaby
Copy link
Contributor Author

It's probable that there are some non-DPI aware APIs being called in the debug text renderer that I didn't catch. 😞

Possible. But this code is strange in that Cyan created it to be DPI aware - but it may have never really worked. AFAIK the DPI check would never have been tested at anything but the default 96 DPI.

@dpogue
Copy link
Member

dpogue commented Mar 20, 2023

Alternatively, maybe we write an implementation of this using freetype2 so that it's cross-platform? (Since I'm going to end up needing to do that for OpenGL anyways, if someone wants to do that work for me... 😛)

@colincornaby
Copy link
Contributor Author

Alternatively, maybe we write an implementation of this using freetype2 so that it's cross-platform? (Since I'm going to end up needing to do that for OpenGL anyways, if someone wants to do that work for me... 😛)

I feel like you might have just volunteered yourself. 😛

With FreeType2 we'd still want to make sure that everything lines up the same which might be tricky. I'm getting the sense a lot of this font code just happened to work and had some amount of magic numbers, which is being upset by the DPI work. I cleaned a lot of that up on the renderer side. And while I don't see any magic numbers in the font map code - something is clearly up.

@colincornaby
Copy link
Contributor Author

Update on where I am with this:

There are issues with how the text rendering code was originally written. Plasma wasn't intended to be DPI aware when it shipped, so they're understandable.

I'm going to grab this graphic from Stackoverflow to demonstrate the issue:

TrueType fonts have layout metrics that define the layout of the glyphs. This spacing is not guaranteed to be stable between font sizes or faces.

Plasma lays out text from the top of the character. But Courier New specifically does not have consistent top padding between font sizes for its glyphs. This is causing the text's offset to shift between font sizes.

A fix would be using the text baseline (the green line) for text layout. This line is stable in relation to the glyphs and would be more predictable between font sizes. Unfortunately, this requires passing through additional metrics about the font and re-writing some rendering code.

I have most the work done, but right now I'm just making sure I didn't break the standard DPI on Windows.

@colincornaby colincornaby force-pushed the backing-scale-changes branch 2 times, most recently from 4bb5dc8 to c25f554 Compare April 12, 2023 05:20
@@ -136,23 +157,57 @@ uint16_t *plTextFont::IInitFontTexture()
// Loop through characters, drawing them one at a time
RECT r;
r.left = r.top = 0;
r.right = r.bottom = 10;
r.right = 1;
r.bottom = 10;
FillRect( hDC, &r, (HBRUSH)GetStockObject( GRAY_BRUSH ) );

// (Make first character a black dot, for filling rectangles)
SetPixel( hDC, 0, 0, RGB( 255, 255, 255 ) );
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does anyone know what lines 158 through 165 are about? The first character is going to be a space, which feels sufficient. I have no idea what this other code is doing. @dpogue @Hoikas

Copy link
Member

Choose a reason for hiding this comment

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

That looks a little bit like it's drawing a cursor, assuming you mean:

    RECT    r;
    r.left = r.top = 0;
    r.right = r.bottom = 10;
    FillRect( hDC, &r, (HBRUSH)GetStockObject( GRAY_BRUSH ) );

    // (Make first character a black dot, for filling rectangles)
    SetPixel( hDC, 0, 0, RGB( 255, 255, 255 ) );

Not sure about the SetPixel() call though.

@colincornaby colincornaby force-pushed the backing-scale-changes branch 3 times, most recently from 9049200 to c41e984 Compare April 17, 2023 05:13
@colincornaby colincornaby force-pushed the backing-scale-changes branch 2 times, most recently from 2b611b3 to 5dcb92f Compare April 28, 2023 00:34
Sources/Plasma/Apps/plClient/CMakeLists.txt Outdated Show resolved Hide resolved
@@ -3337,6 +3337,7 @@ bool plDXPipeline::CaptureScreen( plMipmap *dest, bool flipVertical, uint16_t d
}
else
{
// FIXME: DPI awareness
Copy link
Member

Choose a reason for hiding this comment

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

This function is never used anywhere, so omitting should be fine for now, IMO.

Sources/Plasma/PubUtilLib/plPipeline/CMakeLists.txt Outdated Show resolved Hide resolved
Sources/Plasma/PubUtilLib/plPipeline/plTextFont.cpp Outdated Show resolved Hide resolved
Sources/Plasma/PubUtilLib/plPipeline/plTextFont.cpp Outdated Show resolved Hide resolved
Sources/Plasma/PubUtilLib/plPipeline/plTextFont.cpp Outdated Show resolved Hide resolved
nHeight = -MulDiv( fSize, GetDeviceCaps( hDC, LOGPIXELSY ), 72 );
fFontHeight = -nHeight;

int nHeight = -MulDiv( fSize, plWinDpi::Instance().GetDpi(), 72);
Copy link
Member

Choose a reason for hiding this comment

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

Is this ever re-done when the DPI changes while the game is running?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

DPI change is treated as a device reset IIRC just like any other resolution change - which should flush textures like this one.

I don't know that this path has ever been fully tested however.

colincornaby and others added 2 commits June 25, 2023 13:27
Co-authored-by: Adam Johnson <AdamJohnso@gmail.com>
@dpogue dpogue linked an issue Jun 25, 2023 that may be closed by this pull request
@Hoikas
Copy link
Member

Hoikas commented Jun 25, 2023

Still waiting on the update to cmake/Dependencies.cmake, then we should be good to go.

@colincornaby
Copy link
Contributor Author

Still waiting on the update to cmake/Dependencies.cmake, then we should be good to go.

Not seeing that comment, guessing it's just buried in the others. But I'm guessing the change you'd like to see is FreeType moving to required from optional?

@Hoikas
Copy link
Member

Hoikas commented Jun 25, 2023

Yes, per @dpogue

@dpogue dpogue merged commit 45f3fa2 into H-uru:master Jun 26, 2023
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Patcher/Launcher banner does not compensate for Scaling Garbled Text when System Scaling >= 300%
4 participants