Metal Rendering on macOS #1326

colincornaby · 2023-02-17T06:08:44Z

Update:

Welcome back! Now that the Mac client is merged, we're just down to the Metal bits.

There are still some major things missing from this PR that people should be aware of. This will be a milestone - but not quite the endpoint yet for work.

Resolution switching does not work in this PR. There is an attempt to implement this in Virtual Display Mode Support #1244. Some things are going to be wonky because the client doesn't know the current display mode. The Mysterium demo included Virtual Display Mode Support #1244.
The Mac client cannot patch itself and uses the Windows patch data. Being dealt with elsewhere.
There are some build issues that can be worked around on Apple Silicon.
Cross compiling between architectures still does not work.
The shaders are direct ports of the HLSL 1.1 assembly. They're not written for readability - they're meant to give the exact same results as DX. I have a branch going with ongoing work to sanitize this code. The code also backs off of exact numerical matching and implements things like fast sin instead of Cyan's sin approximation code. That may not end up as part of this PR.

This PR is very much a draft. Getting this out there so people can look and give early feedback. A few known things:

Formatting is a bit of a mess. Still looking at Clang format to clean things up. Some of this was written before I understood the projects conventions.
The Mac client code itself is a bit of a mess. It still needs more cleanup.
There is a shader cache subsystem that I'm not completely happy with. I haven't quite figured out how to clean it up. It's a giant hash table that takes shader attributes and retrieves/caches the right shader program based on that. I think maybe it could be done smarter. And possibly also have a shared implementation with GL.
The Mac client can't do executable patching yet.
There's no launcher with the Mac client. It's all one app. So far - this is intended.
The Mac client can't patch unless the account has admin privs. There is no privilege escalation built in.
GPU based character skinning is not yet in this branch (looking more likely I'll pull it in.)
Resolution controls and frame buffer scaling is not yet in this branch, so Retina support is still shaky. (Parts of this are being considered on the backing-scale-changes branch.

There are probably more things I've forgotten as well. Again - early draft PR.

colincornaby · 2023-02-17T06:09:35Z

I'm also fine with breaking this PR down into smaller chunks where possible.

colincornaby · 2023-02-17T06:12:41Z

Sources/Plasma/FeatureLib/pfMetalPipeline/plMetalPipelineState.h

+
+#include "plMetalDevice.h"
+#include "plSurface/plShaderTable.h"
+


This entire thing is the shader program cache that I am very much not in love with. These structures are hashable representations of different pipeline states. This is to prevent lurching as new materials are loaded. If a program suitable for rendering a material was already created, it's just reused.

This also could mean that an initial set of common programs could be precompiled before the age even loads. In practice, there aren't very many.

dpogue · 2023-02-17T06:26:32Z

Sources/Plasma/FeatureLib/pfMetalPipeline/plMetalEnumerate.mm

+        @autoreleasepool {
+            NSProcessInfo *processInfo = [NSProcessInfo processInfo];
+            NSOperatingSystemVersion version  = processInfo.operatingSystemVersion;
+            NSString *versionString = [NSString stringWithFormat:@"%li.%li.%li", (long)version.majorVersion, (long)version.minorVersion, version.patchVersion];
+            devRec.SetDriverVersion([versionString cStringUsingEncoding:NSUTF8StringEncoding]);
+        }
+        devRec.SetDriverDesc(device->name()->utf8String());


Any chance we could use hsSystemInfo::GetOperatingSystem() here (and avoid contaminating our C++ code with Objective C)?

It would give slightly different output ("macOS 11.0" vs "11.0.0") but should still be reasonable?

Yeah, I don't think that would be an issue. I don't think this string is used for anything critical?

I'm not sure Obj-C++ is the worst thing, but I understand the hesitation.

Ooooof. hsSystemInfo::GetOperatingSystem() actually uses private API. I'd discourage its use until it can be fixed. That would be a blocker if the iOS App Store was ever a thing (which I know is a longer conversation.) In since there is a working implementation it might be better to stick with that for now. I can also look at rolling GetOperatingSystem() over to public API. That might require some Obj-C++ though.

Regarding the private APIs, we discussed this a bit when hsSystemInfo was originally added: #1014 (comment)

/System/Library/CoreServices/SystemVersion.plist exists on both macOS and iOS.

NSProcessInfo is the definitive way to do this under "modern" Cocoa. However, I'd still want to do some amount of digging before making a decision. IGetAppleVersion could be moved to Obj-C++.
https://developer.apple.com/documentation/foundation/nsprocessinfo/1410906-operatingsystemversion

Stuff like _kCFSystemVersionProductNameKey is actively being rejected by Apple. Not necessarily the highest priority in since this isn't going through the App Store (right now.) But I also wouldn't want to leave a risk like that lurking in the code for someone else to stumble upon.

dpogue · 2023-02-17T06:28:04Z

Sources/Plasma/FeatureLib/pfMetalPipeline/plMetalDevicePerformanceShaders.mm

+    fCurrentRenderTargetCommandEncoder = nil;
+
+    //look up the shader by sigma value
+    MPSImageGaussianBlur *blur = (MPSImageGaussianBlur *)fBlurShaders[sigma];


Shame this stuff isn't part of metal-cpp (or even the metal-cpp extensions for Apple's demo projects) 😞

Also a good reminder for me that these shader programs aren't properly lifetime managed with the device.

dpogue

We maybe want to rename the plClient subfolder from OSX to macOS since it's not 2014 anymore and they're changed their branding slightly.

There are a few parts of this that might conflict with #1325 or #1201 where the exact same changes have happened in the same files, but those should be easy enough to resolve.

I think we still want to have a discussion around how metal-cpp gets included. I don't think we love the idea of bundling it, so an automatic CMake fetch from a known URL might be the least bad option.

Sources/Plasma/PubUtilLib/plGImage/plMipmap.cpp

Sources/Plasma/PubUtilLib/plAudio/plAudioSystem_Private.h

dgelessus

Some things (mostly minor) that I noticed while looking over the non-graphics bits...

CMakeLists.txt

Sources/Plasma/Apps/plClient/CMakeLists.txt

Sources/Plasma/Apps/plClient/OSX/PLSKeyboardEventMonitor.mm

Sources/Plasma/Apps/plClient/OSX/PLSLoginWindowController.mm

dgelessus · 2023-02-18T18:16:12Z

Sources/Plasma/Apps/plClient/OSX/PLSView.mm

+    plIMouseXEventMsg* pXMsg = new plIMouseXEventMsg;
+    plIMouseYEventMsg* pYMsg = new plIMouseYEventMsg;


These could be local variables instead of allocated via new, right?

We did that in the Windows client, but there might be some threading going on here for macOS

This did used to be threaded on macOS - but I reverted it back to a main thread only model. There were some weird side effects going on. It could move back to a render thread model in the future though. I'll take a look at this chunk though.

Looking at this. In plMouseDevice these are also being created as pointers, and the function for sending the mouse message expects a pointer.

Sources/Plasma/FeatureLib/inc/pfAllCreatables.h

Sources/Plasma/PubUtilLib/plNetClientComm/plNetClientComm.cpp

Sources/Plasma/Apps/plClient/CMakeLists.txt

dpogue · 2023-02-18T20:11:01Z

Sources/Plasma/Apps/plClient/CMakeLists.txt

+        MACOSX_BUNDLE_BUNDLE_VERSION "0.1"
+        MACOSX_BUNDLE_SHORT_VERSION_STRING "0.1"


These (in theory) should match the values we use for the plProduct stuff:

Plasma/CMakeLists.txt

Lines 25 to 32 in 2457af4

set(PRODUCT_BRANCH_ID "1" CACHE STRING "Branch ID")

set(PRODUCT_BUILD_ID "918" CACHE STRING "Build ID")

set(PRODUCT_BUILD_TYPE "50" CACHE STRING "Build Type")

set(PRODUCT_CORE_NAME "UruLive" CACHE STRING "Product Core Name")

set(PRODUCT_SHORT_NAME "UruLive" CACHE STRING "Product Short Name")

set(PRODUCT_LONG_NAME "Uru Live" CACHE STRING "Product Long Name")

set(PRODUCT_UUID "ea489821-6c35-4bd0-9dae-bb17c585e680"

CACHE STRING "Product UUID")

MACOSX_BUNDLE_BUNDLE_VERSION should likely be some sort of build version. It's supposed to always increment so I'd have to figure out how to combine the data into a number that always goes up.

For MACOSX_BUNDLE_SHORT_VERSION_STRING - do we have an actual version number thats derived anywhere?

https://github.com/H-uru/Plasma/blob/master/cmake/BuildInfo.cmake is used to embed git information into the executable, but that's not currently available in project mode.

dpogue · 2023-02-18T20:14:09Z

Sources/Plasma/Apps/plClient/CMakeLists.txt

+    target_link_libraries(plClient PRIVATE "-framework Cocoa")
+    target_link_libraries(plClient PRIVATE "-framework QuartzCore")
+    target_link_libraries(plClient PRIVATE "-framework MetalPerformanceShaders")


These can probably be specified below with the rest of the target_link_libraries using syntax like $<$<PLATFORM_ID:Darwin>:"-framework Cocoa">

Checking into this. I'd like to figure out if there is a Mac specific platform ID so I can make sure Cocoa is only imported on macOS. (Still thinking about a future iOS build.) There probably is, I just need to look it up.

Sources/Plasma/Apps/plClient/CMakeLists.txt

dpogue · 2023-02-18T20:25:25Z

Sources/Plasma/Apps/plClient/OSX/PLSView.mm

+    plIMouseXEventMsg* pXMsg = new plIMouseXEventMsg;
+    plIMouseYEventMsg* pYMsg = new plIMouseYEventMsg;


We did that in the Windows client, but there might be some threading going on here for macOS

Sources/Plasma/FeatureLib/pfConsole/pfConsoleCommands.cpp

Sources/Plasma/NucleusLib/pnInputCore/plInputMap.cpp

colincornaby · 2023-02-19T07:49:09Z

Main.mm isn't usually supposed to be this loaded in a Cocoa app. It would probably be good to start factoring things out into Cocoa classes.

colincornaby · 2023-02-20T03:04:26Z

I think we still want to have a discussion around how metal-cpp gets included. I don't think we love the idea of bundling it, so an automatic CMake fetch from a known URL might be the least bad option.

I had the same thought. I'll think through if there are any better options. I'm not entirely sure if Apple's URLs are guaranteed to be stable.

colincornaby · 2023-02-26T20:50:06Z

Renamed the "OSX" directory to "Mac-Cocoa."

Very likely if this ever makes it to iPad, I'll end up writing a new client in SwiftUI. "Mac-Cocoa" signifies a Mac Client written in Cocoa/AppKit. This leaves space for an "Apple-SwiftUI" client or something later.

Hoikas

16/172 files viewed

Sources/Plasma/Apps/plClient/CMakeLists.txt

Hoikas · 2023-02-26T21:34:11Z

Sources/Plasma/Apps/plClient/CMakeLists.txt

+        MACOSX_BUNDLE_BUNDLE_VERSION "0.1"
+        MACOSX_BUNDLE_SHORT_VERSION_STRING "0.1"


https://github.com/H-uru/Plasma/blob/master/cmake/BuildInfo.cmake is used to embed git information into the executable, but that's not currently available in project mode.

Sources/Plasma/Apps/plClient/CMakeLists.txt

Sources/Plasma/Apps/plClient/Mac-Cocoa/PLSKeyboardEventMonitor.mm

colincornaby · 2023-03-02T18:16:55Z

Also a reminder that I'm not sure that the Mac keyboard event code works properly with internationalized keyboards. There's only a keymap for English. I remember a while ago there was discussion about USB HID keyboard ids. Mac is currently using the Mac key codes with its own lookup tables between the Plasma and Mac codes.

Apple's Game Controller library also incudes keyboard support meant specifically for games - and it's possible that it would have a cleaner integration path. But that API is only available from macOS 11 onwards. The existing Mac key codes system should probably be sufficient for now. An iPad port would likely implement Game Controller keyboard integration - and might share that implementation with newer versions of macOS.

dgelessus · 2023-03-02T22:39:36Z

Also a reminder that I'm not sure that the Mac keyboard event code works properly with internationalized keyboards. There's only a keymap for English.

I haven't tested the Mac version recently, but the code looks to me like it should work properly regardless of what keyboard layout you're using (as long as it has Latin letters on it). PLSKeyboardEventMonitor passes both the key code (layout-neutral) and the character (layout-dependent) on to plInputManager::HandleKeyEvent, and from there on everything should behave the same as on Windows.

colincornaby · 2023-03-05T01:40:58Z

macOS builds coming out of CI aren't viable right now. Not sure why. They're missing all the embedded libraries.

colincornaby · 2023-03-05T01:46:56Z

Actually - I might be wrong. Does the Github Actions/vcpkg workflow link everything statically? I'm used to seeing dynamic libraries but it might just be that static linking is occurring.

dpogue · 2023-03-05T01:51:23Z

We try to do static linking with vcpkg to cut down on the number of DLLs that need to be shipped for Windows clients.

colincornaby · 2023-03-05T02:11:44Z

Python might be a bit of a problem. I think we've avoided dealing with it so far. Dynamically linked Python included all of the Python .so files in the app bundle, so they were all signed and trusted.

Statically linked Python puts them all in the Python/system folder. This means they're not signed, so the app won't link to them. It's also kind of discouraged to link outside your app bundle, but not impossible.

Would it be possible to have the Mac version build against and embed Python.framework? I'm also not sure what the role of Python.pak is and if any executables live in there.

colincornaby · 2023-03-05T02:16:16Z

This also might be of interest, but is outside the vcpkg ecosystem:

https://github.com/beeware/Python-Apple-support

colincornaby · 2023-08-15T02:19:33Z

@dpogue - Guessing that you'd like to see the Metal-cpp headers moved out of tree. I'll start thinking about what to do. In since it's a header library I don't know that it's critical that it should be managed elsewhere. But as third party code it does make reviews messy.

…venting a delegate message from properly setting Plasma resolution on launch.

…e callback (cherry picked from commit bcb7266f29fc71b136ce9c973fe41aac445ebfec)

Sources/Plasma/Apps/plClient/Mac-Cocoa/main.mm

The render command encoder is create lazily - and clear is not a draw call in Metal. A clear only pass meant no render was being encoded for the command buffer. The render command encoder management could use a bit of cleaning - but for now manually forcing the lazy creation of a command encoder. This could be probably moved to a non-lazy model - but it’s lazy right now because we’re holding for a clear command.

Hoikas

Grass.metal is missing the CWE license block
Metal files still have code style issues, mostly around stray whitespace and brackets needing to be on a new line.
CMakeLists.txt line 62 spurious whitespace around parens
CMakeLists.txt FetchContent call - indents are 4 spaces
plMetalDeviceRef.h 292 zero initialization doesn't need to be explicit
plMetalMaterialShaderRef::EncodeArguments std::function parameters should probably be const references.
plMetalPipeline only base class destructors should be marked virtual.
Spurious whitespace in plMetalPipeline.h
plMetalPlateManager.cpp 52 zero initialization doesn't need to be explicit

Hoikas

It looks like 7848944 messed up the whitespace in the license block of Avatar.metal.

A bit of strategy - do we want to merge #1284 first, remove the .clang-format files from this PR, then merge this one?

colincornaby · 2023-11-27T04:55:54Z

I updated the clang-format file in #1284 so it could be merged first.

Hoikas · 2023-11-28T00:48:43Z

Ok, the .clang-format file is in, now that #1284 is merged. I think that, once the .clang-format files are culled from this PR, I will be 👍 on this. Then, we can start refining stuff in future PRs.

Hoikas

If anyone has any concerns about merging this as-is, please say so ASAP. The mac and metal work is not complete, but this, IMO, will serve as a good baseline for continuing work.

dpogue

I'm 👍 on merging this, and sorting further things out in followup PRs.

I'll also rebase the OpenGL work after this is merged and see if I can consolidate anything.

Hoikas · 2023-11-29T03:12:48Z

💥 💥 💥

colincornaby commented Feb 17, 2023

View reviewed changes

dpogue reviewed Feb 17, 2023

View reviewed changes

Sources/Plasma/PubUtilLib/plGImage/plMipmap.cpp Outdated Show resolved Hide resolved

Sources/Plasma/PubUtilLib/plAudio/plAudioSystem_Private.h Outdated Show resolved Hide resolved

dgelessus reviewed Feb 18, 2023

View reviewed changes

dpogue reviewed Feb 18, 2023

View reviewed changes

colincornaby force-pushed the apple-metal branch 2 times, most recently from 698daf0 to ca7b78b Compare February 19, 2023 07:27

Hoikas reviewed Feb 26, 2023

View reviewed changes

colincornaby force-pushed the apple-metal branch from daef67c to b75b85b Compare February 28, 2023 01:30

colincornaby force-pushed the apple-metal branch from b560588 to 060b280 Compare March 4, 2023 05:37

colincornaby force-pushed the apple-metal branch from 6e7052d to 3824c52 Compare March 7, 2023 04:06

colincornaby force-pushed the apple-metal branch from a6534a0 to cebc394 Compare June 26, 2023 04:41

dgelessus mentioned this pull request Jul 13, 2023

Bare apple client #1420

Merged

colincornaby force-pushed the apple-metal branch 2 times, most recently from f3e4d1e to 4c91c8a Compare August 15, 2023 02:09

colincornaby changed the title ~~macOS Client + Metal Rendering~~ Metal Rendering on macOS Aug 15, 2023

colincornaby marked this pull request as ready for review August 15, 2023 02:11

colincornaby added 13 commits November 13, 2023 19:29

Removing delegate message filter for window size change. This was pre…

d9f0cb2

…venting a delegate message from properly setting Plasma resolution on launch.

First set of feedback changes

9568467

Another set of changes

8dab1e3

More feedback implementation

0705ab6

Work on sanitizing use of sizeof and buffer sizes

ea39b7e

Removing manual include of headers that are already public

168277f

Moving MPS link to public definition in Metal pipeline

f65a5eb

Implementing more review feedback

d662e4c

Shader CMake cleanup

b4899d7

Code feedback

da43aff

Giving renderer direct access to the Metal layer and removing drawabl…

db3c6d4

…e callback (cherry picked from commit bcb7266f29fc71b136ce9c973fe41aac445ebfec)

Indentation fixes for Obj-C pipeline files and Cyan header for shader

1570887

More cleanup

986e8d3

colincornaby force-pushed the apple-metal branch from 57297c1 to 986e8d3 Compare November 14, 2023 03:48

Fixes to fullscreen behavior on startup for Mac

7b548f4

dpogue reviewed Nov 17, 2023

View reviewed changes

Sources/Plasma/Apps/plClient/Mac-Cocoa/main.mm Show resolved Hide resolved

Sources/Plasma/Apps/plClient/Mac-Cocoa/main.mm Outdated Show resolved Hide resolved

colincornaby added 3 commits November 17, 2023 21:40

Removing setting of NSWindow for client window

f3d9764

Using stringWithSTString to set window title

e34919f

Hoikas reviewed Nov 25, 2023

View reviewed changes

Hoikas reviewed Nov 26, 2023

View reviewed changes

Review feedback updates

6c96be0

colincornaby force-pushed the apple-metal branch from 7848944 to 6c96be0 Compare November 27, 2023 04:47

Removing clang format file

245b00f

Hoikas approved these changes Nov 29, 2023

View reviewed changes

dpogue approved these changes Nov 29, 2023

View reviewed changes

Hoikas merged commit 6491a18 into H-uru:master Nov 29, 2023
17 checks passed


		#include "plMetalDevice.h"
		#include "plSurface/plShaderTable.h"

		plIMouseXEventMsg* pXMsg = new plIMouseXEventMsg;
		plIMouseYEventMsg* pYMsg = new plIMouseYEventMsg;

		MACOSX_BUNDLE_BUNDLE_VERSION "0.1"
		MACOSX_BUNDLE_SHORT_VERSION_STRING "0.1"

	set(PRODUCT_BRANCH_ID "1" CACHE STRING "Branch ID")
	set(PRODUCT_BUILD_ID "918" CACHE STRING "Build ID")
	set(PRODUCT_BUILD_TYPE "50" CACHE STRING "Build Type")
	set(PRODUCT_CORE_NAME "UruLive" CACHE STRING "Product Core Name")
	set(PRODUCT_SHORT_NAME "UruLive" CACHE STRING "Product Short Name")
	set(PRODUCT_LONG_NAME "Uru Live" CACHE STRING "Product Long Name")
	set(PRODUCT_UUID "ea489821-6c35-4bd0-9dae-bb17c585e680"
	CACHE STRING "Product UUID")

Metal Rendering on macOS #1326

Metal Rendering on macOS #1326

Conversation

colincornaby commented Feb 17, 2023 • edited Loading

colincornaby commented Feb 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dgelessus Feb 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dpogue left a comment

Choose a reason for hiding this comment

dgelessus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colincornaby commented Feb 19, 2023

colincornaby commented Feb 20, 2023

colincornaby commented Feb 26, 2023

Hoikas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colincornaby commented Mar 2, 2023

dgelessus commented Mar 2, 2023

colincornaby commented Mar 5, 2023

colincornaby commented Mar 5, 2023

dpogue commented Mar 5, 2023

colincornaby commented Mar 5, 2023

colincornaby commented Mar 5, 2023

colincornaby commented Aug 15, 2023

Hoikas left a comment

Choose a reason for hiding this comment

Hoikas left a comment • edited Loading

Choose a reason for hiding this comment

colincornaby commented Nov 27, 2023

Hoikas commented Nov 28, 2023

Hoikas left a comment

Choose a reason for hiding this comment

dpogue left a comment

Choose a reason for hiding this comment

Hoikas commented Nov 29, 2023

colincornaby commented Feb 17, 2023 •

edited

Loading

dgelessus Feb 19, 2023 •

edited

Loading

Hoikas left a comment •

edited

Loading