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

Fix incorrect screen resolution when dpi is set #131

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

HO-COOH
Copy link
Contributor

@HO-COOH HO-COOH commented Feb 21, 2022

Continuation of #126
Close #76

Copy link
Member

@rashil2000 rashil2000 left a comment

Choose a reason for hiding this comment

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

A few comments, mostly for consistency and uniformity with info_disk function.

Can you also use/extend the WinAPI namespace as is done in the info_disk function?

winfetch.ps1 Outdated Show resolved Hide resolved
winfetch.ps1 Outdated Show resolved Hide resolved
winfetch.ps1 Outdated Show resolved Hide resolved
winfetch.ps1 Outdated Show resolved Hide resolved
@HO-COOH HO-COOH requested a review from rashil2000 February 24, 2022 14:55
@rashil2000
Copy link
Member

rashil2000 commented Feb 25, 2022

I'm seeing quite a bit of performance regression (on a laptop with AC plugged in, warm cache):

Before

image

image

After

image

image


This will become more magnified on battery power and further on power saver modes.

@rashil2000
Copy link
Member

I'm not well-versed with C#, so I'll ping @AwesomeCronk (who wrote the info_disk function) to have another set of eyes look at this. Any possible optimizations?

@HO-COOH
Copy link
Contributor Author

HO-COOH commented Feb 25, 2022

Maybe you could try merging those two C# code segments into one, so it compiles in one pass. On my machine I didn't see any significant difference after merging them tho. The C# code itself cannot be significantly improved from what I know, the Win32 APIs are the only solution to this and it's as fast as it can get through PInvoke.

@rashil2000
Copy link
Member

I don't think merging is the solution for this. In the above screenshot I disabled all segments and only tested the resolution segment.

Moreover people would need to disable disk and resolution segments independently, so merging their codes would not be very efficient.

What happens if we move the string processing part out of the C# segment? (Note that the info_disk function manipulates strings in PowerShell itself, the C# part is only for the API calls).

@HO-COOH
Copy link
Contributor Author

HO-COOH commented Feb 25, 2022

Umm yeah, I agree not merging them.

I comment out the returning of strings in GetResolution and just return void and there is almost no difference. String processing shouldn't have any noticeable effect on performance (I tend to believe C# should be faster than powershell anyways). You can also test it by copying the C# code, adding a Main() method that prints the string and time it, it takes about half of the time as in powershell on my machine.

@rashil2000 rashil2000 requested a review from jcwillox February 25, 2022 19:01
@rashil2000
Copy link
Member

I just noticed a bug in this PR:

image

For every repeated call of the script, resolution array is duplicated.

@HO-COOH
Copy link
Contributor Author

HO-COOH commented Apr 12, 2022

👍 fixed

Carterpersall added a commit to Carterpersall/winfetch that referenced this pull request Sep 20, 2022
- Gets the current resolution scale by getting DisplayTransferCharacteristic from WmiMonitorBasicDisplayParams
  - Divides by 96 to get the scale from the given PPI
  - ~2x Slower than the current (but broken) implementation w/o PowerShell bottleneck
    - 34 ms -> 84 ms
  - ~2.7x Faster than lptstr#131 w/o PowerShell bottleneck
    - 230 ms
Carterpersall added a commit to Carterpersall/winfetch that referenced this pull request Sep 22, 2022
- Added two methods of getting screen resolution
  - First one runs if running as admin, and gets the screen information from HKLM:\SYSTEM\CurrentControlSet\Control\GraphicsDrivers\Configuration\
    - Location contains refresh rate, so I added that in too
  - Second method uses implementation in lptstr#131 which is about 4x slower for me, but seems to be the only way to get it working that I can find
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.

Wrong screen resolution reported when display scaling is not at 100%
2 participants