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

[4.x] Add ARCore support #77559

Closed
wants to merge 1 commit into from

Conversation

paddy-exe
Copy link
Contributor

@paddy-exe paddy-exe commented May 27, 2023

This is an updated version for ARCore support of #47455 but for 4.x.

This is a WIP and feedback is very much appreciated

Procedure for building:

Generally follow this build instruction: https://github.com/GodotVR/godot_arcore

My 4.x branch of godot_arcore: https://github.com/paddy-exe/godot_arcore/tree/4.x

NDK version: 21.4.7075529
Android Studio Electric Eel (latest archived version)

Build the updated engine

  1. Clone the repository with the necessary changes:
git clone --branch 4.x-arcore-support git@github.com:paddy-exe/godot.git
  1. Build the engine
cd godot
scons platform=<PLATFORM>
  1. Generate the extension files
cd bin
./godot.<PLATFORM>.editor.x86_64 --dump-extension-api --dump-gdextension-interface

Build the Plugin

  1. Clone the repository of the plugin:
cd ../..
git clone --branch 4.x git@github.com:paddy-exe/godot_arcore.git
  1. Download the godot-cpp submodule
cd godot_arcore
git submodule update --init --recursive
  1. Copy the extension files to the godot-cpp submodule:
cd ..
cp godot/bin/extension_api.json godot/bin/gdextension_interface.h godot_arcore/plugin/libs/godot-cpp/gdextension/ 
  1. Generate the binding classes
cd godot_arcore
python ./generate.py
  1. Build the plugin
./gradlew :generatePluginBinary   

@BastiaanOlij
Copy link
Contributor

Looking good so far, I wonder if the ARCore mode will still be needed now that we can append the needed manifest items through the ARCode AAR, but the camera server changes are definately needed.

@paddy-exe paddy-exe force-pushed the 4.x-arcore-support branch 2 times, most recently from d6ee0b1 to 2361fb6 Compare June 5, 2023 14:49
@dsnopek
Copy link
Contributor

dsnopek commented Jun 9, 2023

Thanks for picking up this effort!

I only skimmed the code, but for the most part the changes seem fairly straight-forward! The only thing I don't understand is: what is the "ARCore" XR mode for?

My 4.x branch of godot_arcore: https://github.com/paddy-exe/godot_arcore/tree/4.x

Your branch appears to have vanished!

@paddy-exe
Copy link
Contributor Author

paddy-exe commented Jun 9, 2023

Thanks for picking up this effort!

I only skimmed the code, but for the most part the changes seem fairly straight-forward!

Thanks for taking a look David!

The only thing I don't understand is: what is the "ARCore" XR mode for?

I am still new at the codebase but as far as I understand it myself Godot doesn't know which XR functionality you want to use (you can use OpenXR on Android as well for VR purposes) on export so that is why you have to tell Godot yourself by setting it in the export settings.

My 4.x branch of godot_arcore: https://github.com/paddy-exe/godot_arcore/tree/4.x

Your branch appears to have vanished!

Should be there again. I seem to have accidentally deleted it on the server-side. Had my local git backup luckily 🍀

Maybe you can also give your input here @dsnopek: In 3.x there was godot_android.h in GDNative. In 4.x the same functionality is in the JavaGodotWrapper class which is not exposed to GDExtension. We need this class to create an environment, get the devices activitiy and more. The question is how it should be done. @BastiaanOlij also mentioned that we could do it in Java.

@paddy-exe paddy-exe force-pushed the 4.x-arcore-support branch 3 times, most recently from 4d7ef13 to ce264b7 Compare June 14, 2023 22:51
@dsnopek
Copy link
Contributor

dsnopek commented Jun 16, 2023

Discussed at the GDExtension meeting: as far as we understand it, the methods and data that would be exposed would only be useful for GDExtension, and so it probably doesn't make sense to expose via ClassDB, where GDScript would have access to it. Instead, it might make more sense to add new functions to the GDExtension interface.

@paddy-exe paddy-exe force-pushed the 4.x-arcore-support branch 2 times, most recently from 6bccfc9 to 05c768f Compare June 18, 2023 21:20
@m4gr3d
Copy link
Contributor

m4gr3d commented Jun 18, 2023

I think it would be helpful to take a step back and clarify what we actually need in order to get ARCore working on Godot 4.x.

Maybe you can also give your input here @dsnopek: In 3.x there was godot_android.h in GDNative. In 4.x the same functionality is in the JavaGodotWrapper class which is not exposed to GDExtension. We need this class to create an environment, get the devices activitiy and more. The question is how it should be done. @BastiaanOlij also mentioned that we could do it in Java.

As @BastiaanOlij mentioned, most of the changes (asides from the camera related ones), and discussed functionality can instead be accessed through Godot Android plugin.

@BastiaanOlij
Copy link
Contributor

As @BastiaanOlij mentioned, most of the changes (asides from the camera related ones), and discussed functionality can instead be accessed through Godot Android plugin.

I think the main issue @paddy-exe is currently running into is not having access to the JavaVM. Those were exposed to GDNative through that class. I don't think that is the way it should be done now, but the correct way of accessing this information is unclear.

I vaguely remember we did this somehow in early ports of the OpenXR plugin but I can't find any info on that anymore.

@paddy-exe
Copy link
Contributor Author

@m4gr3d I am totally fine with doing it either way. I discussed a possibility to create a base class with @dsnopek and create an inheriting class for GDExtension access. I am totally fine with doing it either way. I discussed a possibility to create a base class with @dsnopek and create an inheriting class for GDExtension access. As Bastiaan already mentioned I have no clear understanding how to do this now with the current system.

@dsnopek
Copy link
Contributor

dsnopek commented Jun 19, 2023

@paddy-exe:

I discussed a possibility to create a base class with @dsnopek and create an inheriting class for GDExtension access. I am totally fine with doing it either way.

I'm not sure how the base class fits in anymore, after what we discussed at the GDExtension meeting. I had understood the base class idea to be a way to more cleanly expose JavaGodotWrapper via ClassDB. But as @vnen astutely brought up, this probably shouldn't be exposed to GDScript (which it would be if exposed via ClassDB), only GDExtension, which can be done by adding some new functions to gdextension_interface.h. In that case, you could maybe leave JavaGodotWrapper as it is, and then these new functions can just grab the data you need from it?

If you need help getting that going, feel free to ping me on RocketChat :-)

@paddy-exe
Copy link
Contributor Author

paddy-exe commented Jun 19, 2023

I'm not sure how the base class fits in anymore, after what we discussed at the GDExtension meeting. I had understood the base class idea to be a way to more cleanly expose JavaGodotWrapper via ClassDB. But as @vnen astutely brought up, this probably shouldn't be exposed to GDScript (which it would be if exposed via ClassDB), only GDExtension, which can be done by adding some new functions to gdextension_interface.h. In that case, you could maybe leave JavaGodotWrapper as it is, and then these new functions can just grab the data you need from it?

Ah alright. Yeah the audio during the meeting wasn't great for me due to connection issues. I guess it would be quite hard to do this via the low level gdextension_interface.h.

If you need help getting that going, feel free to ping me on RocketChat :-)

I will revert the last commit and try to get into the Android ARCore plugin and expose the functions there like @m4gr3d suggested and if that fails will come back to your offer. Thanks!

@paddy-exe paddy-exe force-pushed the 4.x-arcore-support branch 2 times, most recently from 9adc1e6 to fd58ad1 Compare June 19, 2023 21:44
@paddy-exe
Copy link
Contributor Author

Alright, reverted back to original commit and supplied the updated CameraFeed.xml so CLI builds successfully without errors.

@paddy-exe paddy-exe force-pushed the 4.x-arcore-support branch from fd58ad1 to 1d1d7bb Compare June 19, 2023 23:08
@paddy-exe paddy-exe force-pushed the 4.x-arcore-support branch from 017551f to fe7a6e4 Compare July 3, 2023 21:21
@paddy-exe paddy-exe requested a review from m4gr3d July 10, 2023 18:42
paddy-exe added a commit to paddy-exe/godot_arcore that referenced this pull request Jul 16, 2023
This commit implements the needed methods for the ARCoreInterface class such as 
* `get_activity()`
* `get_surface()`
* `is_activity_resumed()`
* `get_display_rotation()` -> will be added in ongoing upstream PR: godotengine/godot#77559
@Colbix
Copy link

Colbix commented Dec 21, 2023

Is there any further progress for the ARCore plugin? It looks like you were making headway and only pending a plugin finalize from the 4.x refactoring.

@paddy-exe
Copy link
Contributor Author

paddy-exe commented Dec 22, 2023

Is there any further progress for the ARCore plugin? It looks like you were making headway and only pending a plugin finalize from the 4.x refactoring.

I am still interested in getting this going. I was finalising my thesis and had no time until now. I will try to dedicate some time to this next year hopefully👍🏻

@BastiaanOlij
Copy link
Contributor

Looking at this during our ARCore meeting, this will likely be able to be closed, camera feed logic is being implemented in #95187, display rotation can now be accessed directly in the plugin and no longer needs changes in Godot, but we're unsure about the ARCore mode.

@m4gr3d do you know if the ARCode xr-mode would still be needed or that what Luca does here is enough?

@dsnopek
Copy link
Contributor

dsnopek commented Nov 7, 2024

Is anything in this PR still needed anymore? Or, is it superseded by #96705?

@paddy-exe
Copy link
Contributor Author

Is anything in this PR still needed anymore? Or, is it superseded by #96705?

I don't think so. Will close the PR then

@paddy-exe paddy-exe closed this Nov 7, 2024
@AThousandShips AThousandShips removed this from the 4.x milestone Nov 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants