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

Display Helpers #1566

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

Conversation

colincornaby
Copy link
Contributor

Looking for feedback on this concept. Direct3D is funny in that it directly includes display mode queries as part of its API. For Metal - and OpenGL particularly - things are a bit more interesting. The display mode API is separate from the graphics API.

  • Platforms that support multiple renderers may need to share display mode code between the renderers. I.E. Metal and OpenGL on Mac should share the same display mode code for maintainability.
  • Some renderers support multiple platforms - and need a maintainable way of abstracting the display mode selection by platform. I.E. OpenGL needs to select display modes on Mac, Windows, and Linux.
  • Some platforms have oddities that require deeper interaction with platform native API. I.E. macOS has non rectangular displays, and we need to filter down the available display modes to only that ones that fit on the screen.
  • The client itself needs some sort of interaction with this class. It manages the window - and thus knows what display the window is on and when the window changes displays.

This PR adds display helpers as a structure to try to bridge a specific renderer to a platform specific implementation.

I'm not sure how this fits with enumerations - it's possible that this integration needs to happen at a deeper level in the code.

Comment on lines +83 to +90
// aspect ratio is good - add the mode to the list
plDisplayMode plasmaMode;
plasmaMode.ColorDepth = 32;
plasmaMode.Width = int(CGDisplayModeGetWidth(mode));
plasmaMode.Height = int(CGDisplayModeGetHeight(mode));

fDisplayModes.push_back(plasmaMode);
Copy link
Member

Choose a reason for hiding this comment

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

Should probably use emplace_back() to construct in place (avoids copying).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looking at this - we'd need to define a constructor on plDisplayMode. There is an alternative - but I'm not sure it avoids a copy.

// aspect ratio is good - add the mode to the list
        fDisplayModes.emplace_back() = {
            int(CGDisplayModeGetWidth(mode)),
            int(CGDisplayModeGetHeight(mode)),
            32
        };

FYI - C++20 emplace_back can initialize structs directly without a constructor.

@@ -357,6 +367,9 @@ class plPipeline : public plCreatable

float fBackingScale = 1.0f;
void SetBackingScale(float scale) { fBackingScale = scale; };

std::shared_ptr<plDisplayHelper> fDisplayHelper;
Copy link
Member

Choose a reason for hiding this comment

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

Probably want a better ownership model than shared pointer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Both the client and the pipeline need access to the display helper. I could roll this back so that the reference is owned by the client.

@dpogue
Copy link
Member

dpogue commented Feb 20, 2024

A few high-level thoughts:

  • plPipeline::GetSupportedDisplayModes appears to only be called from the Python code for enumerating the resolution slider options, so we could probably drop the ColorDepth entirely because I don't think that shows up in the options.

  • I guess historically this lived in the pipeline stuff because DirectX conflates them, but maybe it should actually be part of plClient instead? It seems a lot more closely related to the platform-specific client and windowing stuff

  • There's some stuff about display modes in and around the hsG3DDeviceSelector classes used at pipeline creation time, but maybe we don't actually need any of that there and can leave that focused just on renderer device enumeration?

@colincornaby
Copy link
Contributor Author

Digging this PR back out - @dpogue I think you are right. One issue is that the client makes a lot of decisions about resolution - but it does it before the pipeline is necessarily available.

One possible issue is the divergent views on if resolution selection is part of the graphics API or the platform API. Apple feels that it's platform API, but Microsoft sees it as a graphics API issue at least under D3D9.

Other pipelines like OpenGL/Vulkan could cause further chaos, especially if you had one accelerator that supported one set of APIs and another adapter that supported a different set.

There are probably Win32 APIs to map a display to resolutions that I'm not aware of. That might be a good solution. But the display helper still includes some graphics API related code to check capabilities.

@colincornaby
Copy link
Contributor Author

There's also overlap going on between this display helper class and DeviceEnumerator.

@dpogue
Copy link
Member

dpogue commented Aug 6, 2024

I have mostly avoided thinking about this because (as you mentioned) with OpenGL is becomes a complicated nightmare that can't really be done at the driver level.

I agree with the idea that the display mode stuff should be at the plClient/windowing level rather than the pipeline level, although I see how we might still want to filter that list of modes based on capabilities of the pipeline render device.

The original Direct3D renderer used exclusive fullscreen, which presumably is where it's important to use the driver to enumerate the supported modes. We replaced that with fake-fullscreen though, so it might be less important to use that now vs using standard Win32 APIs to ask for the supported resolutions.

I'm imagining something like plClient::GetAvailableDisplayResolutions() being a per-platform implementation that fills in part of the hsG3DDeviceSelector stuff that gets passed in to the pipeline constructor, and then plPipeline::GetSupportedDisplayModes returning a filtered/constrained list based on the capabilities of the active rendering device. Does that sound like it would work?

@colincornaby
Copy link
Contributor Author

I'm imagining something like plClient::GetAvailableDisplayResolutions() being a per-platform implementation that fills in part of the hsG3DDeviceSelector stuff that gets passed in to the pipeline constructor, and then plPipeline::GetSupportedDisplayModes returning a filtered/constrained list based on the capabilities of the active rendering device. Does that sound like it would work?

We might need a display handle to go along with that (along with a re-review of where else this concept is already encapsulated in the pipeline.) Resolutions are going to be per display - and it would be good to clean that whole thing up while we're putting in new plumbing.

The client will probably need to vend which display the window is currently on, and broadcast any changes.

plClient does querying of the possible display modes in it’s constructor. For now - using a singleton as a way to set a global display helper before this process starts.

Keeping display helpers in the plPipeline family. It’s possible it belongs at a lower level alongside hsG3DDeviceMode in since display helpers are platform and not render specific.
@colincornaby
Copy link
Contributor Author

Thinking this through (and the memory lifecycle issues)...

I've baked the concept of display helpers into the app as another object needed by the pipeline. But another option is that I push it through as a hsWindowHndl via SetClientDisplay and act as if the display helper type is the display.

Downside is that the client itself wouldn't be clear on the meaning of the hsWindowHndl (which so far seems to be mostly just used by Windows.) We wouldn't know what it should be cast to. But things like the Metal pipeline could assume.

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.

3 participants