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

Implementing GetMonitorWidth/Height for DRM #3956

Merged
merged 2 commits into from
May 7, 2024

Conversation

gabriel-marques
Copy link
Contributor

Hello :)

I would like to propose an implementation for the following functions for the DRM platform :

  • int GetMonitorWidth(int monitor)
  • int GetMonitorHeight(int monitor)
  • int GetMonitorPhysicalWidth(int monitor)
  • int GetMonitorPhysicalHeight(int monitor)
  • const char *GetMonitorName(int monitor)

Although the monitor selection is not supported for the drm platform I kept the argument to avoid having multiple function prototypes.

I hope it complies to your coding rules :)

…t for drm

Added implementation for DRM for functions :
 - GetMonitorWidth()
 - GetMonitorHeight()
 - GetMonitorPhysicalWidth()
 - GetMonitorPhysicalHeight()
 - GetMonnitorName()

These functions take an argument but only the value 0 is accepted. This is because the DRM platform implementation manages only one screen for now
@colesnicov
Copy link

Hello. What would it be good for? In addition to the fact that I can find out these dimensions... At the moment we have encountered such an unpleasant dilemma regarding DRM: #3946. Can you please tell me about it?

Copy link
Contributor

@orcmid orcmid left a comment

Choose a reason for hiding this comment

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

In GetMonitorName(int monitor)

for consistent behavior with the unimplemented case, you should use

const char name = "";

Also, you might want to do something about the non-zero value case, providing something like "unknown" or "unavailable". This will also avoid compiler warnings about monitor not being used :).

It looks like it might be valuable to have a GetMonitorCount() or similar as part of raylib.h and have that always return 1 for DRM.

Isn't there a raylib.h way to GetCurrentMonitor() or something like that? (I am being lazy.)

@colesnicov
Copy link

I dont know yet. I just tuk here for few days. tests, tests, tests...

@gabriel-marques
Copy link
Contributor Author

Hello @orcmid :)

Thanks for the suggested changes of consistency, I will apply them :)
By the way, there is both GetMonitorCount() and GetCurrentMonitor() which although log that they are not implemented, returns the correct value (because the actual implementation is limited to one screen only)

// Get number of monitors
int GetMonitorCount(void)
{
    TRACELOG(LOG_WARNING, "GetMonitorCount() not implemented on target platform");
    return 1;
}

// Get number of monitors
int GetCurrentMonitor(void)
{
    TRACELOG(LOG_WARNING, "GetCurrentMonitor() not implemented on target platform");
    return 0;
}

@gabriel-marques gabriel-marques marked this pull request as draft May 2, 2024 23:21
@raysan5
Copy link
Owner

raysan5 commented May 5, 2024

@gabriel-marques Implementation looks good to me. Please, let me know when this PR is ready for merging!

Refactored GetMonitorHeight, GetMonitorWidth, GetMonitorPhysicalHeight,
GetMonitorPhysicalWidth and GetMonitorName to accept only argument "0"
as more than one screen is not supported in DRM platform.
@gabriel-marques
Copy link
Contributor Author

Hello,

As @orcmid suggested I changed the getMonitorName to return "" as default and also I changed others functions to also evaluate negative monitors.

@gabriel-marques gabriel-marques marked this pull request as ready for review May 7, 2024 08:03
@raysan5 raysan5 merged commit fa2b1c8 into raysan5:master May 7, 2024
@raysan5
Copy link
Owner

raysan5 commented May 7, 2024

@gabriel-marques Thanks! Implementation looks good to me! Merging!

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.

4 participants