Update graphics contract, Update to latest version of BabylonNative #191
Conversation
… update-graphics-contract
…e UpdateView contract
…e UpdateView contract
…eactNative into update-graphics-contract
| void Initialize(facebook::jsi::Runtime& jsiRuntime, Dispatcher jsDispatcher); | ||
| void Deinitialize(); | ||
| void UpdateView(void* windowPtr, size_t width, size_t height, void* windowTypePtr = nullptr); | ||
| void UpdateView(void* windowPtr, size_t width, size_t height); |
There was a problem hiding this comment.
Ideally, we should be removing void * from this header as well, in favor of WindowType.
Currently I cannot include the headers here without build failure, I'll keep tooling with CMake to so that this works, but it may require making this a library instead of a collection of source files included in another library. I'm no CMake expert, however.
There was a problem hiding this comment.
id suggest redefining the type here or using a void pointer for the time being. We can update include directories for the native modules to track down the headers required to access the window type definition. But right now the native modules aren't precompiled binaries when distributed through npm. So if BabylonNative.h references babylon native headers and our react-native native module references BabylonNative.h, i'm pretty sure wed have to package and distribute all headers required to resolve the windowtype, which is going to be a lot more work/seems beyond the scope of fixing breaking changes.
There was a problem hiding this comment.
After chatting offline, I'll go forward with retaining void*.
| "${CMAKE_CURRENT_LIST_DIR}/DispatchFunction.h" | ||
| "${CMAKE_CURRENT_LIST_DIR}/BabylonNative.h" | ||
| "${CMAKE_CURRENT_LIST_DIR}/BabylonNative.cpp") | ||
| "${CMAKE_CURRENT_LIST_DIR}/BabylonNative.cpp") |
| { | ||
| GraphicsConfiguration graphicsConfig = GraphicsConfiguration(); | ||
| graphicsConfig.WindowPtr = (WindowType)windowPtr; | ||
| graphicsConfig.Width = width; |
There was a problem hiding this comment.
nit can we have this as a reinterpret_cast()?
| { | ||
| ANativeWindow* windowPtr{ ANativeWindow_fromSurface(env, surface) }; | ||
| ANativeWindow* windowPtr = ANativeWindow_fromSurface(env, surface); | ||
| auto width{ static_cast<size_t>(ANativeWindow_getWidth(windowPtr)) }; |
There was a problem hiding this comment.
nit: was this change needed?
There was a problem hiding this comment.
not at all, reverting
|
Looks like everything's green outside of the Android build failures |
| nvtt.lib; | ||
| OGLCompilerd.lib; | ||
| openxr_loader.lib; | ||
| openxr_loaderd.lib; |
There was a problem hiding this comment.
Aren't these changes part of @chrisfromwork's PR? Not sure why they are showing up here.
There was a problem hiding this comment.
These changes were recommended by chris, I added them to this PR so that it would pass windows CI. We can check in #188 and I can rebase, else we pretty much have the same goal here, I'm just integrating my changes from the new latest commit.
There was a problem hiding this comment.
yeah in moving to the newest master we'll see breaks in ci without this change
|
@ryantrem, are we good to check in? |
…Platform.h include
Co-authored-by: Ryan Tremblay <ryantrem@msn.com>
3 participants
This PR integrates changes from BabylonJS/BabylonNative#582, updating the BabylonNative graphics initialization code to consume the configuration-based contract.