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

Add screen_get_refresh_rate to DisplayServer #47246

Closed

Conversation

jordi-star
Copy link
Contributor

@jordi-star jordi-star commented Mar 22, 2021

Closes godotengine/godot-proposals#1284.

Allows you to get the Screen's refresh rate by using DisplayServer.screen_get_refresh_rate()

image
image

Since 3.2 doesn't use DisplayServer, I would imagine this can't be cherry-picked. I've already made a 3.x version that uses OS instead. I will make a separate PR for the 3.x branch if this gets merged.

@jordi-star jordi-star force-pushed the os-screen-refresh-rate-4 branch 4 times, most recently from 848ceb2 to 021a154 Compare March 22, 2021 03:33
@jordi-star jordi-star force-pushed the os-screen-refresh-rate-4 branch from 021a154 to e81ad86 Compare March 22, 2021 03:47
@jordi-star jordi-star marked this pull request as ready for review March 22, 2021 04:24
@jordi-star jordi-star requested review from a team as code owners March 22, 2021 04:24
@jordi-star jordi-star removed request for a team March 22, 2021 04:25
@jordi-star
Copy link
Contributor Author

Added documentation and fixed issues with the method being missing in iOS and Android builds.

@@ -138,6 +138,11 @@ Size2i DisplayServerAndroid::screen_get_size(int p_screen) const {
return OS_Android::get_singleton()->get_display_size();
}

float DisplayServerAndroid::screen_get_refresh_rate(int p_screen) const {
// Most Android devices have 60hz displays.
Copy link
Member

Choose a reason for hiding this comment

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

Quick search on gsmarena.com and, half of the last year released €200+ devices have 90+ Hz screens.

https://developer.android.com/reference/android/view/Display#getMode()
https://developer.android.com/reference/android/view/Display.Mode#getRefreshRate()

@@ -2285,6 +2285,13 @@ static void displays_arrangement_changed(CGDirectDisplayID display_id, CGDisplay
return Size2i();
}

float DisplayServerOSX::screen_get_refresh_rate(int p_screen) const {
_THREAD_SAFE_METHOD_
// There's no way to get the screen refresh rate using AppKit
Copy link
Member

Choose a reason for hiding this comment

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

Internal displays are 60Hz, external can have any refresh rate.

API to get refresh rate. https://developer.apple.com/documentation/coregraphics/1454661-cgdisplaymodegetrefreshrate

Enumeration example:

	NSArray *screenArray = [NSScreen screens];
	for (int i = 0; i < [screenArray count]; i++) {
		NSDictionary *description = [[screenArray objectAtIndex:i] deviceDescription];
		double rate = CGDisplayModeGetRefreshRate(CGDisplayCopyDisplayMode([[description objectForKey:@"NSScreenNumber"] unsignedIntValue]));
		if (rate == 0) {
			rate = 60.f;
		}
		printf("refresh rate [%d]: %f\n", i, rate);
	}

Copy link
Member

@Calinou Calinou Nov 2, 2021

Choose a reason for hiding this comment

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

Since this was posted, note that internal displays can now be 120 Hz in the new 14" and 16" MacBook Pros. This isn't always the case though, as they can be configured to run at a lower refresh rate to save on battery (as low as 47.95 Hz).

Comment on lines +331 to +339
float DisplayServerWindows::screen_get_refresh_rate(int p_screen) const {
_THREAD_SAFE_METHOD_

DEVMODE data;
memset(&data, 0, sizeof(data));
EnumDisplaySettingsA(NULL, ENUM_CURRENT_SETTINGS, &data);
return data.dmDisplayFrequency;
}

Copy link
Member

Choose a reason for hiding this comment

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

I guess it should get the current display refresh rate, not the main one. Also, setting xxSize field value is usually required for the most Windows API functions.

Something like (not tested):

Suggested change
float DisplayServerWindows::screen_get_refresh_rate(int p_screen) const {
_THREAD_SAFE_METHOD_
DEVMODE data;
memset(&data, 0, sizeof(data));
EnumDisplaySettingsA(NULL, ENUM_CURRENT_SETTINGS, &data);
return data.dmDisplayFrequency;
}
typedef struct {
int count;
int screen;
float rate;
} EnumRefData;
static BOOL CALLBACK _MonitorEnumProcRef(HMONITOR hMonitor, HDC hdcMonitor, LPRECT lprcMonitor, LPARAM dwData) {
EnumRefData *data = (EnumRefData *)dwData;
if (data->count == data->screen) {
MONITORINFOEXW mi;
memset(&mi, 0, sizeof(mi);
mi.cbSize = sizeof(mi);
GetMonitorInfoW(hMonitor, &mi);
DEVMODE dm;
memset(&dm, 0, sizeof(dm));
dm.dmSize = sizeof(dm);
EnumDisplaySettingsW(mi.szDevice, ENUM_CURRENT_SETTINGS, &dm);
data->rate = dm.dmDisplayFrequency;
}
data->count++;
return TRUE;
}
float DisplayServerWindows::screen_get_refresh_rate(int p_screen) const {
_THREAD_SAFE_METHOD_
EnumRefData data = { 0, p_screen == SCREEN_OF_MAIN_WINDOW ? window_get_current_screen() : p_screen, 0 };
EnumDisplayMonitors(nullptr, nullptr, _MonitorEnumProcRef, (LPARAM)&data);
return data.rate;
}

Comment on lines +2288 to +2294
float DisplayServerOSX::screen_get_refresh_rate(int p_screen) const {
_THREAD_SAFE_METHOD_
// There's no way to get the screen refresh rate using AppKit
// and most if not all Apple devices have 60hz displays anyways.
return 60.0;
}

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
float DisplayServerOSX::screen_get_refresh_rate(int p_screen) const {
_THREAD_SAFE_METHOD_
// There's no way to get the screen refresh rate using AppKit
// and most if not all Apple devices have 60hz displays anyways.
return 60.0;
}
float DisplayServerOSX::screen_get_refresh_rate(int p_screen) const {
NSArray *screenArray = [NSScreen screens];
int id = (p_screen == SCREEN_OF_MAIN_WINDOW) ? window_get_current_screen() : p_screen;
NSDictionary *description = [[screenArray objectAtIndex:id] deviceDescription];
CGDisplayModeRef mode = CGDisplayCopyDisplayMode([[description objectForKey:@"NSScreenNumber"] unsignedIntValue]);
double rate = CGDisplayModeGetRefreshRate(mode);
CGDisplayModeRelease(mode);
return (rate > 0) ? rate : 60.f;
}

Copy link
Contributor Author

@jordi-star jordi-star Mar 22, 2021

Choose a reason for hiding this comment

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

Thanks for the review, I'll definitely implement these changes later today. Apologies for the inconvenience.

For now, I'll mark this as a draft until I get everything working.

@jordi-star jordi-star marked this pull request as draft March 22, 2021 13:45
</argument>
<description>
Return's the screen refresh rate.
[b]Note:[/b] On macOS and iPhone, this will always be 60.
Copy link
Contributor

Choose a reason for hiding this comment

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

I would use this is instead of this will always be since it could change in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This line won't be in the final merged version of this PR, since I'm adding support for getting the refresh rate through AppKit

Copy link
Member

@Calinou Calinou Apr 3, 2021

Choose a reason for hiding this comment

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

I would also replace "iPhone" with "iOS", since Godot also supports iPadOS and in the future, tvOS (Apple TV).

The iPad Pro already has a 120 Hz refresh rate.

Edit: The iPhone 13 Pro and iPhone 13 Pro Max have 120 Hz refresh rate as well, unless the battery saver mode is enabled.

platform/android/display_server_android.cpp Outdated Show resolved Hide resolved
//invalid screen?
ERR_FAIL_INDEX_V(p_screen, get_screen_count(), 0);

float refresh_rate = 60.0; //Default to 60 hz if the engine was unable to get the refresh rate.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a place you can store the default value (maybe in display_server.h) and just reference it?

Copy link
Contributor Author

@jordi-star jordi-star Apr 2, 2021

Choose a reason for hiding this comment

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

I would do that, but it looks like most other places that have a default value do it hard coded like this. Plus, I don't see many benefits to adding it to display_server.h since it most likely won't be changed any time soon, and if it is, it would be a very quick change.

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

Successfully merging this pull request may close these issues.

Add an OS method to retrieve a screen's refresh rate
6 participants