Bare apple client #1420

colincornaby · 2023-07-09T21:57:32Z

This is a bare Mac client - stripping out the Metal renderer to try to Mac review more piecemeal. It does not implement GL - if you login the game will work, just without display.

Few notes:

The amount of code in Main.m is very un-Cocoa-like. This should probably be split into different classes - but the initial Mac GL client was used as the starting point. I've attempted to move new code into proper Obj-C classes.
Obj-C classes were created for Mac specific behavior, or when the engine needed to bound to Obj-C code that relies on Obj-C code concepts.
I've removed Metal code everywhere except for PLSView - where it is guarded by a preprocessor flag. I thought this would be helpful for showing how GL needs to be integrated. Going forward - GL probably needs to add a proper GL layer, and GL needs to draw based on the system callback for timing.
The Mac executable still does not self patch and is hard coded with some behaviors to support the Windows manifest.

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

Hoikas · 2023-07-12T22:05:06Z

I do see the request for me to review this. The challenge for me is that I'm really out of my depth here in mac land, but I will do my best to look at this soon-ish.

dgelessus

Some low-hanging fruit I noticed on a semi-quick read...

Scripts/Ports/python3/portfile.cmake

Scripts/Triplets/arm64-osx.cmake

Sources/Plasma/Apps/plClient/Mac-Cocoa/Assets.xcassets/AppIcon.appiconset/UruIcon.png

Sources/Plasma/Apps/plClient/Mac-Cocoa/Info.plist.in

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

Sources/Plasma/CoreLib/HeadSpin.cpp

colincornaby · 2023-07-14T04:10:56Z

I do see the request for me to review this. The challenge for me is that I'm really out of my depth here in mac land, but I will do my best to look at this soon-ish.

Following back up on this - The Obj-C is going to be difficult to get peer review on. But I'm hoping to get some feedback on C++ and CMake bits. My experience with CMake (or vcpkg) is not as deep as yours. So I think even if the Obj-C itself isn't being reviewed by everyone, there are parts of this where I'd like to get feedback from the perspective of the Windows and Linux maintainers.

Hoikas

Considering the terrible implications of things like microsoft/vcpkg#32432, I am wondering if we need to split these ports and our python-cairosvg port out into another repository that acts as a vcpkg registry. That way, we aren't cluttering up our repo with a multitude of supporting vcpkg port tweaks.

Sources/Plasma/Apps/plClient/CMakeLists.txt

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

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

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

Sources/Plasma/CoreLib/CMakeLists.txt

cmake/PlasmaTargets.cmake

Sources/Plasma/Apps/plClient/Mac-Cocoa/.clang-format

colincornaby · 2023-07-20T05:48:30Z

I think we're getting towards the end of the current feedback on this review. Things that still need to be dealt with (and apologies if I forgot feedback on this list, please follow up with things I forgot!):

There are still some comments leftover in the Mac Main from the Windows version when stuff was being copied over. These comments no longer apply and should be deleted.
Anything VCPKG. Builds feel much longer right now. It looks like cpython is being rebuilt every build now.
Something I'm tracking but may not resolve here - the Python/Cairo vcpkg stuff is still sometimes failing on Apple Silicon builds. I believe Cairo is only being used by asset generation bits and not the client. Python still doesn't look in the Homebrew folder location on Apple Silicon for Cairo - so it fails the Cairo test on build. I've been manually patching the correct rpath into Python. But if we're applying diffs to Python anyway, maybe the Homebrew rpath could be added in another diff.

Sources/Plasma/Apps/plClient/plClient.cpp

patmauro

If it's not too much to ask - would you mind co-authoring me in the commit(s) for the icons? Put a lot of work into em and I'm really proud of the results, so I just wanna make sure I have proper attribution there. :) Thanks!

colincornaby · 2023-07-21T04:45:14Z

If it's not too much to ask - would you mind co-authoring me in the commit(s) for the icons? Put a lot of work into em and I'm really proud of the results, so I just wanna make sure I have proper attribution there. :) Thanks!

Sure! I'll make the change to the Git history.

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

colincornaby · 2023-07-23T07:18:06Z

If it's not too much to ask - would you mind co-authoring me in the commit(s) for the icons? Put a lot of work into em and I'm really proud of the results, so I just wanna make sure I have proper attribution there. :) Thanks!

Just following up - I made this change a few days ago. Go ahead and review! Wanted to make sure you were aware.

colincornaby · 2023-07-24T02:24:32Z

I'm considering disabling breaking up long lines in Obj-C files. There was the earlier discussion about long argument sets in Obj-C. But what I'm seeing is that Clang-Format is breaking up some Obj-C lines that only have one or several arguments.

I think it's a consequence of Obj-C function calls just being longer than C++ calls.

colincornaby · 2023-07-24T05:27:36Z

I went ahead and turned off line length enforcement entirely in Obj-C files. That means it's up to the developer to use their best judgement in breaking up Obj-C lines or letting them wrap.

Again - my concern was that a strict line limit was leading to some really poor choices in breaking up Obj-C function calls. It would be great if Clang format could be smarter and set a minimum argument count until it started breaking up function calls. But I haven't seen that option.

patmauro · 2023-07-24T17:58:22Z

If it's not too much to ask - would you mind co-authoring me in the commit(s) for the icons? Put a lot of work into em and I'm really proud of the results, so I just wanna make sure I have proper attribution there. :) Thanks!

Just following up - I made this change a few days ago. Go ahead and review! Wanted to make sure you were aware.

Perfect, thanks. :) Appreciate it!

Sources/Plasma/Apps/plClient/Mac-Cocoa/[email protected]

Sources/Plasma/Apps/plClient/Mac-Cocoa/.clang-format

colincornaby · 2023-07-27T04:09:00Z

Still a few things left here, but I thought I'd mention a few issues that I'm reserving for future pull requests:

Python not having the right rpath to find Cairo from Homebrew on Apple Silicon. In since we're already patching Python anyway this patch could be added.
Something is weird with the post build script and the pngcrush path and I'm seeing it fail out of the box. Don't know if it's just my machine, the Github Actions seem fine. If there is a configuration change required, that will be a separate PR.

colincornaby · 2023-07-27T04:56:41Z

I believe we're all resolved up. So this is either ready to move ahead or ready for more feedback.

vcpkg-configuration.json

Sources/Plasma/CoreLib/HeadSpin_Mac.mm

Sources/Plasma/CoreLib/HeadSpin.cpp

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

Sources/Plasma/Apps/plClient/Mac-Cocoa/.clang-format

Sources/Plasma/Apps/plClient/CMakeLists.txt

colincornaby · 2023-07-30T06:24:55Z

@Hoikas - In since we've been through a round of the Obj-C indentation discussion with @dgelessus - I'm going to go ahead and resolve those conversations. The short version I think we got down to: Obj-C style is really long function names, they can wrap super funny because the : needs to be aligned, we're not sure what the best solution is, but Obj-C won't be forced to indent.

cmake/VcpkgToolchain.cmake

Co-authored-by: Adam Johnson <[email protected]>

Co-authored-by: Pat Mauro <[email protected]>

Co-authored-by: Adam Johnson <[email protected]>

Also leaving notes for future explorers.

There be dragons here. We originally kept Github Actions from performing the zip because our archive had symlinks in it, which it turns into hard copes of files. Xcode's archive and code signing tooling expects those symlinks, so we weren't able to sign. As of right now - there are no symlinks in the archive. So things should be ok. If any frameworks are added this will break again. Hopefully by that point Github has resolved their issues.

dpogue

Aside from this minor nitpick, I think we are at a point where it makes sense to merge this

Sources/Plasma/NucleusLib/pnInputCore/plInputMap.cpp

Hoikas

Just code style and typo nits, I think.

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

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

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

Sources/Plasma/CoreLib/HeadSpin.cpp

Co-authored-by: Adam Johnson <[email protected]>

Hoikas

LGTM. I'd like to hear from @dpogue before merging, however.

dpogue

Let's merge it, and then I'll rebase the Linux client stuff on top.

Worth noting (again, for the record) that this macOS client currently has no renderer, so it will not build a client that "works" for any playable definition of "working".

colincornaby · 2023-08-15T00:32:58Z

Let's merge it, and then I'll rebase the Linux client stuff on top.

Worth noting (again, for the record) that this macOS client currently has no renderer, so it will not build a client that "works" for any playable definition of "working".

I expect I'll have a rebased version of the Metal renderer on top of this shortly after merge.

colincornaby requested review from dpogue and Hoikas July 9, 2023 21:57

Deledrius reviewed Jul 9, 2023

View reviewed changes

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

dgelessus reviewed Jul 13, 2023

View reviewed changes

Hoikas reviewed Jul 15, 2023

View reviewed changes

colincornaby force-pushed the bare-apple-client branch from d61e900 to e255521 Compare July 16, 2023 02:43

colincornaby commented Jul 17, 2023

View reviewed changes

Sources/Plasma/Apps/plClient/Mac-Cocoa/.clang-format Outdated Show resolved Hide resolved

dpogue reviewed Jul 20, 2023

View reviewed changes

Sources/Plasma/Apps/plClient/plClient.cpp Outdated Show resolved Hide resolved

patmauro reviewed Jul 20, 2023

View reviewed changes

colincornaby force-pushed the bare-apple-client branch 3 times, most recently from 0ceca45 to ee85339 Compare July 21, 2023 05:08

dgelessus reviewed Jul 22, 2023

View reviewed changes

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

patmauro reviewed Jul 24, 2023

View reviewed changes

Sources/Plasma/Apps/plClient/Mac-Cocoa/[email protected] Outdated Show resolved Hide resolved

dgelessus mentioned this pull request Jul 25, 2023

Experimental optional CFString/NSString support zrax/string_theory#26

Closed

dgelessus reviewed Jul 25, 2023

View reviewed changes

Sources/Plasma/Apps/plClient/Mac-Cocoa/.clang-format Show resolved Hide resolved

Hoikas reviewed Jul 29, 2023

View reviewed changes

Hoikas mentioned this pull request Jul 29, 2023

Vendor the PhysX 4.1.2 vcpkg port for now #1430

Closed

Hoikas reviewed Jul 31, 2023

View reviewed changes

cmake/VcpkgToolchain.cmake Outdated Show resolved Hide resolved

colincornaby and others added 19 commits August 12, 2023 20:57

Refining Cmake HeadSpin_Mac include

4062894

Adding clang-format specific to Mac client

76fcbae

Cleaning up code formatting in Mac client

73f18ca

Update Sources/Plasma/Apps/plClient/CMakeLists.txt

b74036d

Co-authored-by: Adam Johnson <[email protected]>

Cleanup on HeadSpin_Mac

46189eb

Upadting Mac icon

7869eb2

Updating icon with latest version

034a0ce

Co-authored-by: Pat Mauro <[email protected]>

Changing Mac banner to png

1122989

Column limit in Clang-Format for Obj-C is now at developer discretion

e6ee96e

Updating login banner for Retina with new version

9628eb4

Co-authored-by: Pat Mauro <[email protected]>

Updating Clang Format for left aligned Obj-C ptr

41c18fc

Unifying Obj-C/C++ format settings

88a4dfe

Apply suggestions from code review

36765c4

Co-authored-by: Adam Johnson <[email protected]>

Converting Mac exclude list to set

73302a9

Changing app identifier to H’uru

4eec2f7

Also leaving notes for future explorers.

Removing unused menus and resources

cdadacc

Cleaning up imports

1d54966

Updating input map with Plasma key keys

28091b4

colincornaby force-pushed the bare-apple-client branch from 7a83085 to fd68041 Compare August 13, 2023 03:58

Catching up with skip preload changes

3cfa409

dpogue reviewed Aug 13, 2023

View reviewed changes

Sources/Plasma/NucleusLib/pnInputCore/plInputMap.cpp Outdated Show resolved Hide resolved

Hoikas reviewed Aug 13, 2023

View reviewed changes

colincornaby and others added 2 commits August 13, 2023 18:30

Apply suggestions from code review

9523b6c

Co-authored-by: Adam Johnson <[email protected]>

Re-moving unneeded import

84dfebf

Hoikas approved these changes Aug 14, 2023

View reviewed changes

dpogue approved these changes Aug 14, 2023

View reviewed changes

colincornaby merged commit e9b5125 into H-uru:master Aug 15, 2023
14 checks passed

colincornaby deleted the bare-apple-client branch August 15, 2023 00:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bare apple client #1420

Bare apple client #1420

colincornaby commented Jul 9, 2023

Hoikas commented Jul 12, 2023

dgelessus left a comment

colincornaby commented Jul 14, 2023

Hoikas left a comment

colincornaby commented Jul 20, 2023

patmauro left a comment

colincornaby commented Jul 21, 2023

colincornaby commented Jul 23, 2023

colincornaby commented Jul 24, 2023

colincornaby commented Jul 24, 2023

patmauro commented Jul 24, 2023

colincornaby commented Jul 27, 2023

colincornaby commented Jul 27, 2023

colincornaby commented Jul 30, 2023

dpogue left a comment

Hoikas left a comment

Hoikas left a comment

dpogue left a comment

colincornaby commented Aug 15, 2023

Bare apple client #1420

Bare apple client #1420

Conversation

colincornaby commented Jul 9, 2023

Hoikas commented Jul 12, 2023

dgelessus left a comment

Choose a reason for hiding this comment

colincornaby commented Jul 14, 2023

Hoikas left a comment

Choose a reason for hiding this comment

colincornaby commented Jul 20, 2023

patmauro left a comment

Choose a reason for hiding this comment

colincornaby commented Jul 21, 2023

colincornaby commented Jul 23, 2023

colincornaby commented Jul 24, 2023

colincornaby commented Jul 24, 2023

patmauro commented Jul 24, 2023

colincornaby commented Jul 27, 2023

colincornaby commented Jul 27, 2023

colincornaby commented Jul 30, 2023

dpogue left a comment

Choose a reason for hiding this comment

Hoikas left a comment

Choose a reason for hiding this comment

Hoikas left a comment

Choose a reason for hiding this comment

dpogue left a comment

Choose a reason for hiding this comment

colincornaby commented Aug 15, 2023