Closed colincornaby closed 1 year ago
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.
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.
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!):
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.
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.
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.
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.
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!
Still a few things left here, but I thought I'd mention a few issues that I'm reserving for future pull requests:
I believe we're all resolved up. So this is either ready to move ahead or ready for more feedback.
@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.
Mac client should be settled again. I've brought in @dpogue's vcpkg triplet changes.
Two additional thoughts but neither of them are deal-breakers:
PLS
as a class prefix risks a bit of confusion, because the original codebase usesps
andpls
to refer toplServers
and the (closed-source)ServerLib
folder. My first suggestion would beURU
but we've otherwise tried not to bake the game title into the code.- The
Mac-Cocoa
folder in plClient could be renamedmacOS
ormacos
(which happens to be the same 5-character length aswin32
andlinux
).
On 1: I think I'd be more comfortable with PLS in since this is technically an engine and not Uru. On 2: My feeling is it's better to organize the client folder by framework, and not by platform. There still is a SwiftUI client that will probably get dropped in someday. And while that's intended for iOS and VisionOS, it will also run on Mac and could replace this Mac client. Win32 is technically a framework name. And if there ever was a Qt or .Net client (just tossing random things out there) I would assume those clients would be organized by framework name as well.
Something is weird with the Github actions Mac zip archive. If I download it with Safari it unzips as expected. If I download with Edge on Windows it's a zip inside of a zip. Seems like there is something different about the archive. UruManifest also gets tripped up.
That's because the archive really is a ZIP inside another ZIP. The macOS Archive Utility by default automatically extracts archives recursively, so you don't notice the difference there.
I don't think this is a new thing - I also remember seeing a double ZIP archive when I looked at an older version of the Mac client. I'm guessing that GitHub Actions always puts artifacts inside a ZIP, even if there's only one artifact that's already a ZIP...
I'm not sure why the Linux and macOS builds are now on fire. I just made a minor change to the Github Actions script. Hm.
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.
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: