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.

Sign up for GitHub

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

Jump to bottom

Try moving Metal Layer handling to the pipeline #1554

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

Try moving Metal Layer handling to the pipeline #1554

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
951eb8d
Try moving Metal Layer handling to the pipeline
dpogue Jan 6, 2024
ef04fe5
Make plPipeline::GetLayer const
dpogue Jan 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 6 additions & 27 deletions Sources/Plasma/Apps/plClient/Mac-Cocoa/PLSView.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,6 @@
*==LICENSE==*/

#import "PLSView.h"
#include <Metal/Metal.h>
#include <QuartzCore/QuartzCore.h>
#include "plMessage/plInputEventMsg.h"

Expand All @@ -63,39 +62,16 @@
@interface PLSView ()

@property NSTrackingArea* mouseTrackingArea;
#if PLASMA_PIPELINE_METAL
@property(weak) CAMetalLayer* metalLayer;
#endif

@end

@implementation PLSView

// MARK: View setup
- (id)initWithFrame:(NSRect)frameRect
{
self = [super initWithFrame:frameRect];
#if PLASMA_PIPELINE_METAL
CAMetalLayer* layer = [CAMetalLayer layer];
layer.contentsScale = [[NSScreen mainScreen] backingScaleFactor];
layer.maximumDrawableCount = 3;
layer.pixelFormat = MTLPixelFormatBGR10A2Unorm;
Copy link
Contributor

Choose a reason for hiding this comment

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

Previously we've talked about inspecting the screen bit depth and setting the layer pixel format based on that. I.E. don't set a 10 bit color depth if the screen isn't 10 bit. I don't know if that change is moving ahead - but moving that out of the client would complicate things. The answer might need to be some sort of engine level flag for 10 bit color. In theory - Direct3D 9 supports 10 bit color output as well.

I've also seen Apple just set this to 16 bit in their examples (which is what our fragment shaders output in Metal) and call it a day. But memory usage would be higher.

self.layer = self.metalLayer = layer;
#endif
self.layer.backgroundColor = NSColor.blackColor.CGColor;
return self;
}

- (BOOL)acceptsFirstResponder
{
return YES;
}

- (BOOL)wantsLayer
{
return YES;
}

// MARK: Left mouse button
- (void)mouseDown:(NSEvent*)event
{
Expand Down Expand Up @@ -275,9 +251,12 @@ - (void)resizeDrawable:(CGFloat)scaleFactor
return;
}

#if PLASMA_PIPELINE_METAL
_metalLayer.drawableSize = newSize;
#endif
self.layer.contentsScale = scaleFactor;

if ([self.layer isKindOfClass:[CAMetalLayer class]]) {
((CAMetalLayer*)self.layer).drawableSize = newSize;
Copy link
Member Author

Choose a reason for hiding this comment

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

I wanted to do this with respondsToSelector: and performSelector:withObject: but CGSize isn't an id type so it can't be sent through there. Also can't do setValue:forKey: which was going to be my other workaround, so we get ugly casting for now 😞

NSInvocation would probably work but that looks awful to use

Copy link
Contributor

Choose a reason for hiding this comment

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

NSInvocation is typically on the developer do-not-use list (Swift does not support it in any form) - so NSInvocation should be avoided.

I think the way it's phrased is fine. drawableSize exists on all versions of CAMetalLayer AFAIK so the CAMetalLayer check is sufficient.

}

[self.delegate renderView:self
didChangeOutputSize:newSize
scale:scaleFactor];
Expand Down
55 changes: 29 additions & 26 deletions Sources/Plasma/Apps/plClient/Mac-Cocoa/main.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,9 +42,6 @@

// System Frameworks
#import <Cocoa/Cocoa.h>
#ifdef PLASMA_PIPELINE_GL
#import <OpenGL/gl.h>
#endif
#ifdef PLASMA_PIPELINE_METAL
#import <Metal/Metal.h>
#endif
Expand Down Expand Up @@ -82,18 +79,12 @@
#include "plNetGameLib/plNetGameLib.h"
#include "plProduct.h"

// Until a pipeline is integrated with macOS, need to import the
// abstract definition.
#include "plPipeline/pl3DPipeline.h"

void PumpMessageQueueProc();

extern bool gDataServerLocal;
extern bool gPythonLocal;
extern bool gSDLLocal;

bool NeedsResolutionUpdate = false;

std::vector<ST::string> args;

@interface AppDelegate : NSWindowController <NSApplicationDelegate,
Expand All @@ -111,7 +102,6 @@ @interface AppDelegate : NSWindowController <NSApplicationDelegate,
@property CVDisplayLinkRef displayLink;
@property dispatch_queue_t renderQueue;
@property CALayer* renderLayer;
@property(weak) PLSView* plsView;
@property PLSPatcherWindowController* patcherWindow;
@property NSModalSession currentModalSession;
@property PLSPatcher* patcher;
Expand Down Expand Up @@ -191,16 +181,18 @@ - (id)init
defer:NO];
window.backgroundColor = NSColor.blackColor;

PLSView* view = [[PLSView alloc] init];
self.plsView = view;
window.contentView = view;
[window setDelegate:self];

gClient.SetClientWindow((__bridge void *)view.layer);
PLSView* plView = [[PLSView alloc] init];
window.contentView = plView;

gClient.SetClientWindow((__bridge void *)plView.layer);
gClient.SetClientDisplay((hsWindowHndl)NULL);

self = [super initWithWindow:window];
self.window.acceptsMouseMovedEvents = YES;

plView.delegate = self;
[window setDelegate:self];

return self;
}

Expand All @@ -209,7 +201,7 @@ - (id)init
- (void)startRunLoop
{
[[NSRunLoop currentRunLoop] addPort:[NSMachPort port] forMode:@"PlasmaEventMode"];
[self.plsView setBoundsSize:self.plsView.bounds.size];
Copy link
Contributor

Choose a reason for hiding this comment

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

Odd. But I think maybe this line was here to get the view to just enter setBoundsSize and do setup.

[self.window.contentView setBoundsSize:self.window.contentView.bounds.size];
auto* msg = new plDisplayScaleChangedMsg(self.window.backingScaleFactor);
msg->Send();

Expand Down Expand Up @@ -457,20 +449,27 @@ - (void)startClient
{
PF_CONSOLE_INITIALIZE(Audio)

self.plsView.delegate = self;
self.renderLayer = gClient->GetPipeline()->GetRenderLayer();
_renderLayer.backgroundColor = NSColor.blackColor.CGColor;

self.window.contentView.layer = _renderLayer;
self.window.contentView.wantsLayer = YES;
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure how I feel. PLSView was meant to be self contained and wrap all details about the layer. Mostly.

Copy link
Member Author

Choose a reason for hiding this comment

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

There's some nuance here that I don't entirely understand about layer-hosted views and layer-backed views and the order of setting layer and wantsLayer

// Create a window:

// Window controller
[self.window setContentSize:NSMakeSize(800, 600)];
[self.window center];
[self.window makeKeyAndOrderFront:self];
self.renderLayer = self.window.contentView.layer;

[self.renderLayer addObserver:self
forKeyPath:@"device"
options:NSKeyValueObservingOptionNew | NSKeyValueObservingOptionInitial
context:DeviceDidChangeContext];


#ifdef PLASMA_PIPELINE_METAL
if ([_renderLayer respondsToSelector:@selector(device)]) {
[_renderLayer addObserver:self
forKeyPath:@"device"
options:NSKeyValueObservingOptionNew | NSKeyValueObservingOptionInitial
context:DeviceDidChangeContext];
}
#endif
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a good change. I think that this probably does extra checking. It could simply check the layer type and then check for device - in since CAMetalLayer already has device defined. The respondsToSelector would be unnecessary (wouldn't want some other device function floating in unintentionally in a different layer type.) The ifdef might be redundant in since it's a dynamic check, but we could skip the dynamic check if Metal wasn't present.

Longer term - we need a common code path for GL and Metal to handle screen and device changes. I think screen right now is all CGDisplay based so that's agnostic to the underlying API.

Copy link
Member Author

Choose a reason for hiding this comment

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

As far as I can tell, the only thing this is currently being used for it to print the Metal device name in the window title for debugging purposes, so I'm hopeful it can be dropped entirely at some point in the future.

Copy link
Contributor

Choose a reason for hiding this comment

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

Strangely - this has been useful. On Intel MacBook Pros I've run into situations in which Plasma is running on the wrong GPU, so it's a nice sanity check. However it could be maybe re-implemented against the enumeration code that already exists in the engine and may already be storing the same data.

There's another reason this is a helpful debug hint. Plasma may need to switch GPUs mid run (which can be supported with Plasma's GPU crash recovery mechanisms.) So double checking the GPU device would be helpful for proofing that out. But I haven't gotten eGPU ejection or Plasma-follows-your-current-displays-GPU yet.


if (!gClient) {
exit(0);
}
Expand Down Expand Up @@ -563,7 +562,11 @@ - (void)observeValueForKeyPath:(NSString *)keyPath ofObject:(id)object change:(N

- (void)dealloc
{
[_renderLayer removeObserver:self forKeyPath:@"device" context:DeviceDidChangeContext];
#ifdef PLASMA_PIPELINE_METAL
if ([_renderLayer respondsToSelector:@selector(device)]) {
[_renderLayer removeObserver:self forKeyPath:@"device" context:DeviceDidChangeContext];
}
#endif
}

@end
Expand Down
8 changes: 8 additions & 0 deletions Sources/Plasma/FeatureLib/pfMetalPipeline/plMetalPipeline.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4372,6 +4372,14 @@ uint32_t plMetalPipeline::IGetBufferFormatSize(uint8_t format) const
return size;
}

CALayer* plMetalPipeline::GetRenderLayer() const
{
CA::MetalLayer* layer = CA::MetalLayer::layer();
layer->setPixelFormat(MTL::PixelFormatBGR10A2Unorm);
Copy link
Member Author

Choose a reason for hiding this comment

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

maximumDrawableCount defaults to 3, so there's no need to set that explicitly here.

contentsScale is not exposed as part of CA::MetalLayer's API, but I added setting it whenever it changes to the PLSView handlers (which also set the drawableSize).

Copy link
Contributor

Choose a reason for hiding this comment

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

contentsScale is independent from Metal so it probably should go into the client. contentsScale should apply to both GL and Metal evenly. Or whatever else comes along in the future.


return reinterpret_cast<CALayer*>(layer);
}

void plMetalPipeline::plMetalPipelineCurrentState::Reset()
{
fCurrentPipelineState = nullptr;
Expand Down
1 change: 1 addition & 0 deletions Sources/Plasma/FeatureLib/pfMetalPipeline/plMetalPipeline.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,7 @@ class plMetalPipeline : public pl3DPipeline<plMetalDevice>
int GetMaxAnisotropicSamples() override;
int GetMaxAntiAlias(int Width, int Height, int ColorDepth) override;
void ResetDisplayDevice(int Width, int Height, int ColorDepth, bool Windowed, int NumAASamples, int MaxAnisotropicSamples, bool vSync = false) override;
CALayer* GetRenderLayer() const override;
void RenderSpans(plDrawableSpans* ice, const std::vector<int16_t>& visList) override;
void ISetupTransforms(plDrawableSpans* drawable, const plSpan& span, hsMatrix44& lastL2W);
bool ICheckDynBuffers(plDrawableSpans* drawable, plGBufferGroup* group, const plSpan* spanBase);
Expand Down
11 changes: 10 additions & 1 deletion Sources/Plasma/NucleusLib/inc/plPipeline.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,11 @@ You can contact Cyan Worlds, Inc. by email [email protected]
#define DEFAULT_SHADOWS 0
#define DEFAULT_PLANARREFLECTIONS 0

#if __OBJC__
@class CALayer;
#else
class CALayer;
#endif
Copy link
Contributor

Choose a reason for hiding this comment

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

Now I'm curious, but I bet I know the answer... Another reason CALayer was kept out of the renderer was to prevent Obj-C from creeping in. This forward declaration seems to be part of the abstraction against that. Metal can allocate a CALayer through Metal-cpp. Wouldn't this introduce Obj-C (or, I'm guessing Obj-C msg sending) into the GL renderer itself?

One risk of putting it into the pipeline is that a lot of stuff includes the pipeline, so you risk Obj-C leakage. I'm wondering if maybe there needs to be a secondary class that just pairs pipelines with layers, and that way the layer integration and all the mess that goes with can live outside of the pipeline.

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe this is why I'm playing with subclassing CAOpenGLLayer entirely in C++ using objc runtime APIs (🙊), but in a class that's only included when compiling for Darwin.

The alternative is that the client needs to have a CAOpenGLLayer subclass that knows internal state of the GL pipeline like the GL context object, and that feels like it is prone to breaking due to a fuzzy API boundary between the two (whereas CAOpenGLLayer's expected API is well understood)


struct hsPoint3;
struct hsVector3;
Expand Down Expand Up @@ -354,7 +359,11 @@ class plPipeline : public plCreatable
plDisplayMode fDesktopParams;

virtual size_t GetViewStackSize() const = 0;


#ifdef HS_BUILD_FOR_MACOS
virtual CALayer* GetRenderLayer() const { return nullptr; }
#endif

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