Skip to content
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

Remove AR from core Babylon React Native packages (in favor of plugin model) #597

Closed
nima-ap opened this issue Aug 18, 2023 · 6 comments
Closed
Assignees
Labels
enhancement New feature or request
Milestone

Comments

@nima-ap
Copy link
Contributor

nima-ap commented Aug 18, 2023

Is your feature request related to a problem? Please describe.

Babylon React Native provides a wide array of functionalities, including AR scenes. However, not all applications that might require the core rendering functionalities of Babylon on React Native might utilize advanced functionalities such as rendering an AR scene.

Due to the nature of AR requiring access to additional hardware, such as the camera and motion sensors, even if the application makes no usage of Babylon's AR functionalities, they must take extra steps to ensure adherence to public store policies like those found on Google Play and Apple App Store.
Google is more relaxed about this but Apple requires that you explain camera usage even if the runtime does not use it.

As you can imagine, large apps might not be inclined to request additional permissions or include unnecessary metadata in their manifests that claim they use AR while they don't, and so this can be a serious blocker for usage.

Describe the solution you'd like
All AR functionality and dependencies removed from the core Babylon React Native packages, moved into separate package(s) for use by consumers who wish to utilize that functionality.

This also has additional wins for the core packages including:

  • reduced bundle size
  • removal of peer-dependency to RN Permissions library
@ryantrem
Copy link
Member

This would apply equally to other features that require device permissions, like the the camera polyfill, correct?

Also, we already have an explosion of packages due to splitting the package per RN version and platform. Splitting out feature level packages would make this even worse. In the past, we’ve talked about the idea of having just a single NPM package, and platform specific packages for each platform and RN version that are downloaded at build time. For example, we’d have Maven packages published for Android, Cocoa Pods for iOS, and Nuget for Windows. This would hide the platform and RN version complexities, and would result in just one NPM package. If we have this setup, then splitting out feature level packages seems like it would be more manageable. We just end up with a few npm packages like @babylonjs/react-native, @bsbylonjs/react-native-ar, etc.

Thoughts?

@namluu25
Copy link

can we have a patch package to remove the use of AR since some people like me do not use the AR feature at all? but requires the user to grant the camera permission when not using it is not fine at all

@thomlucc thomlucc added this to the 7.0 milestone Aug 24, 2023
@thomlucc thomlucc added enhancement New feature or request bug Something isn't working and removed enhancement New feature or request labels Aug 24, 2023
@CedricGuillemet
Copy link
Contributor

CedricGuillemet commented Sep 4, 2023

I don't see a short term solution except releasing a new specific package without AR/Camera. This would add a new package per RN version and platform.
I believe this change is one of many other linked needs. Like, as said Ryan, reducing the package count explosion, reducing package size (limit is 200Mb, Windows package size is >150Mb for 1 RN version) but also reducing build times (BabylonNative static libraries are built once per RN version for example)
It's not possible to increase package size with different RN version, it's not possible to have more plugins without increasing build times, etc ...
Having BabylonNative prebuilt would help reducing the build times but some libraries depending on JS engine need to be built specifically (see BabylonJS/BabylonNative#1270 )
This looks like weeks or months of work. I guess we need a short term solution before.

EDIT:
I think we discussed distributing source code (or at least the commands to download sources) and build BN/BRN along the app. what was its cons?

EDIT2:
After looking at the build pipeline, it looks to me easier to add a new package for ios-android and windows, maybe just for a few RN versions that will contain just the engine without Camera polyfill or XR. We need to discuss the steps but that new package can be the default one and xr/camera plugins be optional is other repo/npm packages.

@CedricGuillemet CedricGuillemet added enhancement New feature or request and removed bug Something isn't working labels Sep 6, 2023
@CedricGuillemet
Copy link
Contributor

CedricGuillemet commented Nov 14, 2023

I'm thinking about a simple way to get this plugin model to work.
My idea is to have XR and camera code in main NPM. But it would not compile and link unless there is some other npm installed.
Main NPM contains a file EnableXR.h that is empty. XR NPM contains a header named the same but containing #define EnableXR
Depending on include paths order, 1 or the other is included.
In the same vain, in the cmakelists.txt, if directory exists for XR plugin package, list of libraries to link are changed.
On Windows, as it's using a vcxproj, this can be done with a #pragma comment(lib.... in the EnableXR.h header

@bbowman
Copy link

bbowman commented Nov 14, 2023

@CedricGuillemet I think having 'latent' code in the main package is totally fine, especially if its in a separate dll / so that can just be probed for at runtime.

My concern though is using a separate NPM package to control the linking and includes. Maybe this is easiest but I also wonder if that point, there should just be documentation on the main NPM package on what environment variables or config settings (maybe at build time you could read from something?) to control this as opposed to having another (empty) package be this config value

@thomlucc thomlucc modified the milestones: 7.0, 8.0 Apr 2, 2024
@bghgary bghgary modified the milestones: 8.0, Future Apr 16, 2024
@bghgary
Copy link
Contributor

bghgary commented Sep 10, 2024

Closing in favor of #614

@bghgary bghgary closed this as not planned Won't fix, can't repro, duplicate, stale Sep 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

7 participants