-
Notifications
You must be signed in to change notification settings - Fork 19
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
feat: Add macOS support #123
base: main
Are you sure you want to change the base?
Conversation
611c8f2
to
0bea4ac
Compare
@wcandillon Does this package require Hermes? I was getting issues running with JSC. Once #120 lands, I can rebase this PR and see if I'm still seeing those errors :) |
Got it working on macOS with an additional ifdef to get |
@wcandillon Could you let me know if this runs on your machine? I just rebased off of main. I'm happy to split up some of the JS changes / "IOS" -> "Apple" renames to separate PRs if you prefer smaller changes too. |
934877e
to
43322d5
Compare
I see something with @react-three/fiber is failing, I'm trying #140 to see if it helps. |
7448407
to
21aad00
Compare
@wcandillon Any concerns with the current approach here? |
@Saadnajmi I like it a lot thanks for doing it. Anything missing? would you like to add a build step for it on the ci.yml? |
@Saadnajmi I'm not able to compile the project (reanimated and gesture-handler are not compiling, am I missing something? |
OK, I thought I fixed that in React Native macOS, perhaps we need to update to the latest patch version. Let me try locally. |
e3e20cf
to
81e5501
Compare
looks like you need to update the turbo.json and the package.json at the root for build:macos to work |
I think "react-native build-macos" isn't part of the CLI. I can't remember if we have "react-native-macos build-macOS" either. There's some outstanding CLI work we need to do to make sure our commands are registered (ever since the React Native Community CLI got decoupled from React Native) so this PR might get blocked on that :/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for the update. it looks like the MetalView needs to be updated with the following:
#if !TARGET_OS_OSX
- (void)willMoveToSuperview:(UIView *)newSuperview {
[super willMoveToSuperview:newSuperview];
if (newSuperview == nil) {
// The view is being removed from its superview
// Add your cleanup code here
[SurfaceUtils cleanupSurface:[_contextId intValue]];
_isConfigured = NO;
}
}
#else // macOS specific lifecycle method
- (void)viewWillMoveToSuperview:(NSView *)newSuperview {
[super viewWillMoveToSuperview:newSuperview];
if (newSuperview == nil) {
// The view is being removed from its superview
// Add your cleanup code here
[SurfaceUtils cleanupSurface:[_contextId intValue]];
_isConfigured = NO;
}
}
#endif
Gesture handler is not working for me but I guess this is expected? If yes that's more than fine. We could work around it.
The examples where not working for me but also I am not sure how you were able to build the app or run the examples? I would be interested to learn more here.
Could you look at globalSetup.ts
and update the platform checks: https://github.com/wcandillon/react-native-webgpu/blob/main/packages/webgpu/src/__tests__/globalSetup.ts#L14
apps/example/package.json
Outdated
"build:android": "cd android && ./gradlew assembleDebug --warning-mode all", | ||
"build:ios": "yarn run mkdist && react-native bundle --entry-file index.js --platform ios --dev true --bundle-output dist/main.ios.jsbundle --assets-dest dist && react-native build-ios --scheme Example --mode Debug --extra-params \"-sdk iphonesimulator CC=clang CPLUSPLUS=clang++ LD=clang LDPLUSPLUS=clang++ GCC_OPTIMIZATION_LEVEL=0 GCC_PRECOMPILE_PREFIX_HEADER=YES ASSETCATALOG_COMPILER_OPTIMIZATION=time DEBUG_INFORMATION_FORMAT=dwarf COMPILER_INDEX_STORE_ENABLE=NO\"", | ||
"build:macos": "yarn run mkdist && react-native bundle --entry-file index.js --platform macos --dev true --bundle-output dist/main.macos.jsbundle --assets-dest dist", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
kudos :))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that doesn't seem to build the app, could we put the xcode build command here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think I need to actually go and make sure that shows up in the React Native community CLI :D. I'll follow up on that, and meanwhile split out some more changes from this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I checked with @tido64. We need to go and implement build-macos
. We have run-macos
... so it shouldn't be too hard. But it might be a bit.
@Saadnajmi Thank you for this. I will try it. I see that the CI is failing now? |
EDIT: microsoft/react-native-macos#2225 |
Add macOS support to
react-native-webgpu
. This is done by:ios/*
toapple/*
RNWPUIKit.h
that adds a typedef forNSView
andUIView
so we have less ifdefs in the codebase (Reanimated and GestureHandler do the same thing)IOSPlatformContext
toApplePlatformContext
@react-navigation/native-stack
to@react-navigation/stack
as the former does not have macOS support.