-
Notifications
You must be signed in to change notification settings - Fork 60
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
Add UWP support to BabylonReactNative #137
Conversation
Modules/@babylonjs/react-native/windows/BabylonReactNative/EngineViewManager.cpp
Outdated
Show resolved
Hide resolved
Modules/@babylonjs/react-native/windows/BabylonReactNative/EngineViewManager.h
Outdated
Show resolved
Hide resolved
Modules/@babylonjs/react-native/windows/BabylonReactNative/EngineViewManager.h
Outdated
Show resolved
Hide resolved
Modules/@babylonjs/react-native/windows/BabylonReactNative/EngineViewManager.h
Outdated
Show resolved
Hide resolved
Co-authored-by: Ryan Tremblay <ryantrem@msn.com>
} | ||
}); | ||
|
||
result.Resolve(true); |
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.
Seems like you didn't actually change it though?
winrt::event_token RenderingToken; | ||
}; | ||
RevokerData _revokerData{}; | ||
winrt::BabylonReactNative::EngineView _engineView{ nullptr }; |
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.
The only reason this instance is stored is so that it can be replaced, which destroys the old one and unregisters events, right? If/when in the future we add support for multiple view instances, we'll have to update this, and probably do something more like the OnLoaded
/OnUnloaded
approach I mentioned, but I guess we can just do it this way for now and worry about fixing it up as needed in the future.
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 we can refactor this as wanted. right now it seems like BabylonNative initialization is driven by the BabylonModule initialize call. I wasn't sure if we wanted the BabylonNative initialization/deinitialization to be driven by the SwapChainPanel OnLoaded/OnUnloaded.
FWIW, there are some lifecycle bugs floating around for UWP support. We can address these sorts of changes in these follow up fixes.
const makeUWPProject = async () => { | ||
exec('.\\..\\Modules\\@babylonjs\\react-native\\windows\\scripts\\Setup.bat'); | ||
} | ||
|
||
const buildUWPProject = async () => { | ||
exec('.\\..\\Modules\\@babylonjs\\react-native\\windows\\scripts\\Build.bat'); | ||
} | ||
|
||
const buildUWP = gulp.series(makeUWPProject, buildUWPProject); |
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.
Today we run this gulp build on Mac agents so we can build both iOS and Android, which means this added stuff for UWP will break. I think we are going to need to rework this so that we build Android, iOS, and UWP on different build agents, publish the built stuff as artifacts, and then have a separate stage that pulls the artifacts and packages them all together into the NPM package. Maybe we just save the changes to gulpfile.js for a future 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.
that sounds good. I'll probably go ahead and add the buildUWP gulp task to the exports. But i agree it shouldn't be in the main build task.
Co-authored-by: Ryan Tremblay <ryantrem@msn.com>
Co-authored-by: Ryan Tremblay <ryantrem@msn.com>
namespace winrt::BabylonReactNative::implementation { | ||
EngineView::EngineView() { | ||
_revokerData.SizeChangedRevoker = SizeChanged(winrt::auto_revoke, { this, &EngineView::OnSizeChanged }); | ||
_revokerData.PointerPressedRevoker = PointerPressed(winrt::auto_revoke, { this, &EngineView::OnPointerPressed }); |
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.
For pointer events, rather than subscribing to the evented form, you should instead override the OnPointer
* methods. These are invoked before the evented forms, and provide an option for suppressing the event from being raised (by setting the Handled
property) in cases where that might not be valuable (think not raising PointerPressed
for a button but instead a Click
).
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.
sounds good, I'll add this to the upcoming packaging review.
This pr adds support for react-native-windows. Instructions for building the react-native-windows babylon.js package can be found at Modules@babylonjs\react-native\windows\README.txt
Overview:
TODOs:
Follow up issues to file: