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

[WIP] ARKit for Godot 3.1 #24227

Open
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@BastiaanOlij
Copy link
Contributor

BastiaanOlij commented Dec 8, 2018

This PR sees the introduction of our ARKit interface on iOS. We're about 95% of the way to completion and there is even already an app out on the AppStore using this :)

Anyway this does need a proper example project, there is a simple test project that you can find here: https://github.com/BastiaanOlij/godot3_test_projects/tree/ARKit/ARKit

Since the class is only available on iOS its also not part of the documentation, I need to have a look into working around that.

Todos:

  • move a few changes back into the CameraServer PR
  • find a solution for sending pauze and resume notifications (this will likely be a new PR)
  • make a proper solution for dealing with the dependency on iOS 11 (or make that the minimum iOS for Godot?)
  • implement a reflection probe implementation that uses the interface added to ARKit 2.0 to get real world reflections in materials

Note that there are still a few things to improve on the CameraServer itself and that the PR for @bzztbomb needs to be finished and merged.

@BastiaanOlij

This comment has been minimized.

Copy link
Contributor Author

BastiaanOlij commented Dec 8, 2018

I'm not sure why Github decided to auto request all the reviews btw :)

@groud

This comment has been minimized.

Copy link
Contributor

groud commented Dec 8, 2018

We use Github's file owners feature. It automatically asks review for PR that changes a of "owned" files.

@akien-mga akien-mga added this to the 3.2 milestone Dec 9, 2018

@BastiaanOlij BastiaanOlij force-pushed the BastiaanOlij:arkit31 branch 2 times, most recently from 04a8c76 to ae21d9f Jan 12, 2019

@BastiaanOlij

This comment has been minimized.

Copy link
Contributor Author

BastiaanOlij commented Jan 12, 2019

Loads of thanks to Arjen van Staalduinen who's been testing this on an iPad and emailing me patches to apply. I've not been able to test it out myself due to some hardware issues but this is really starting to work well by the looks of what Arjen has been doing.

@BastiaanOlij

This comment has been minimized.

Copy link
Contributor Author

BastiaanOlij commented Jan 12, 2019

Todos that are left:

  • pretty up some code, there are a few shortcuts in there.
  • add code that prevents iOS 11.0 code running on older devices
  • look into ARKit 2.0 enhancements.

@BastiaanOlij BastiaanOlij referenced this pull request Jan 12, 2019

Closed

[WIP] ARKit support #9967

@BastiaanOlij BastiaanOlij force-pushed the BastiaanOlij:arkit31 branch 2 times, most recently from bcc8dc7 to 94699ea Jan 14, 2019

@BastiaanOlij

This comment has been minimized.

Copy link
Contributor Author

BastiaanOlij commented Jan 21, 2019

Ok, important bit of information we'll eventually need to add into the docs :)

Godot has the ability to fix the orientation for mobile apps with the default set to landscape:
project settings

ArKit does work best with a fixed orientation so this default works best but you can change it to sensor to have the 2D part of the UI match the orientation of the iPhone/iPad.

Whichever you chose, you have to make sure that your xcode setting match else Godot will fix the orientation but ArKit will keep adjusting its orientation with some really funky results. So if you stick to the landscape default make sure your xcode project settings are as follows:
xcode settings

@BastiaanOlij BastiaanOlij force-pushed the BastiaanOlij:arkit31 branch 2 times, most recently from fbe2873 to 33df9b8 Jan 27, 2019

@BastiaanOlij BastiaanOlij requested a review from godotengine/documentation as a code owner Jan 27, 2019

@cbscribe
Copy link
Member

cbscribe left a comment

Minor formatting & style changes.

Show resolved Hide resolved doc/classes/CameraServer.xml Outdated
Show resolved Hide resolved doc/classes/CameraServer.xml Outdated
Show resolved Hide resolved doc/classes/CameraServer.xml Outdated
Show resolved Hide resolved doc/classes/Environment.xml Outdated
Show resolved Hide resolved doc/classes/Environment.xml Outdated

@BastiaanOlij BastiaanOlij force-pushed the BastiaanOlij:arkit31 branch from 33df9b8 to 5bea44b Feb 5, 2019

@BastiaanOlij

This comment has been minimized.

Copy link
Contributor Author

BastiaanOlij commented Feb 5, 2019

@cbscribe forgot to do the fixups sorry, need to do those as part of #10643

@BastiaanOlij

This comment has been minimized.

Copy link
Contributor Author

BastiaanOlij commented Feb 5, 2019

Ok, I'm moving ARKit into a module, easier to turn it on/off. Haven't finished moving it completely yet. Still WIP :)

@BastiaanOlij BastiaanOlij force-pushed the BastiaanOlij:arkit31 branch 3 times, most recently from d46046a to 11585a7 Feb 5, 2019

// Note, this also contains a scale factor which gives us an idea of the size of the anchor
// We may extract that in our ARVRAnchor class
Basis b;
matrix_float4x4 m44 = anchor.transform;

This comment has been minimized.

Copy link
@samgreen

samgreen Feb 24, 2019

Member

These lines are begging for a static inline helper

This comment has been minimized.

Copy link
@BastiaanOlij

BastiaanOlij Feb 28, 2019

Author Contributor

lol, yeah it can be cleaned up a bit, I'll be doing that at the end when everything is working fine. I have a feeling there will be more changes here :)

@BastiaanOlij BastiaanOlij force-pushed the BastiaanOlij:arkit31 branch from 11585a7 to 8d3f7f7 Mar 10, 2019

@BastiaanOlij

This comment has been minimized.

Copy link
Contributor Author

BastiaanOlij commented Mar 10, 2019

Just rebased this and made a few small changes. I'm trying to make the focus in/focus out logic for pausing ARKit go through _notification as the notification system is being called for this but unsuccessfully so far. I'm guessing because ARVR Interfaces aren't nodes but more basic objects. Could use some help with solving this better (also impacts ARCore). @reduz any suggestions here?

@samgreen
Copy link
Member

samgreen left a comment

I reviewed the relevant iOS portions. Everything looks good, but I can't quite understand the camera session stuff. It seems like we're duplicating some work that already exists in a more optimized fashion. Would love your input on this as I probably missed something.

Show resolved Hide resolved drivers/gles3/rasterizer_storage_gles3.cpp Outdated
Show resolved Hide resolved drivers/gles3/shaders/ycbcr.glsl Outdated
Show resolved Hide resolved modules/arkit/arkit_interface.mm Outdated
Show resolved Hide resolved modules/arkit/arkit_interface.mm Outdated
if (plane_detection_is_enabled != p_enable) {
plane_detection_is_enabled = p_enable;

// Restart our session (this will be ignore if we're not initialised)

This comment has been minimized.

Copy link
@samgreen

samgreen Mar 11, 2019

Member

The comment says initalised but the code reads session_was_started. Which is correct?

This comment has been minimized.

Copy link
@BastiaanOlij

BastiaanOlij Mar 27, 2019

Author Contributor

Let me come back to you on that one once I have a moment to catch up :)

This comment has been minimized.

Copy link
@BastiaanOlij

BastiaanOlij Apr 18, 2019

Author Contributor

Ok, so this is some weirdness around how phones work, it doesn't work fully yet because I haven't got notifications to work properly just yet.

Anyway, the problem lies around an app becoming dormant when you switch away from it. Basically ARKit needs to be doing its thing when you've initialized it AND we have focus.

So if ARKit is initialized before we obtain focus, we initialize ARKit but we don't start the session. Then when we obtain focus start_session is called and we start our ARKit session.
If we obtain focus before ARKit is initialized start_session is called but initialize is false so we don't do anything more, the session_was_started ensures that once we initialize we also setup the ARKit session.

If at any time after this has all been setup we loose focus stop_session is called and we simply pause our ARKit session and unpause it in start_session once focus is regained (the unpauze is actually a reinitialize as settings on the phone may have changed).

This comment has been minimized.

Copy link
@samgreen

samgreen Apr 18, 2019

Member

I wonder if we should either hook the appDidResignActive and appDidBecomeActive events from iOS, or if we should hoist them up to Godot and provide other notifications for the window.

b.elements[1].z = m44.columns[2][1];
b.elements[2].z = m44.columns[2][2];
tracker->set_orientation(b);
tracker->set_rw_position(Vector3(m44.columns[3][0], m44.columns[3][1], m44.columns[3][2]));

This comment has been minimized.

Copy link
@samgreen

samgreen Mar 11, 2019

Member

Since we're missing the scale in the transform here, Won't that prevent ARKit from resizing the generated surface to match the environment?

This comment has been minimized.

Copy link
@BastiaanOlij

BastiaanOlij Mar 27, 2019

Author Contributor

That is why we have set_rw_position (rw as in real world), the scale is applied later. This so that if the position of an anchor doesn't change, but you change the scale, we still get everything positioned correctly

This comment has been minimized.

Copy link
@BastiaanOlij

BastiaanOlij Apr 18, 2019

Author Contributor

Hmm, now that I think of it, we do need to apply our scale to the mesh somehow or it will have the wrong size, need to think that one through... That might need to be solved in the anchor.tscn

Show resolved Hide resolved modules/arkit/arkit_interface.mm
Show resolved Hide resolved modules/arkit/arkit_session_delegate.h
Show resolved Hide resolved platform/iphone/camera_ios.mm
@@ -142,6 +142,7 @@ def configure(env):
'-framework', 'CoreAudio',
'-framework', 'CoreGraphics',
'-framework', 'CoreMedia',
'-framework', 'CoreVideo',

This comment has been minimized.

Copy link
@samgreen

samgreen Mar 11, 2019

Member

Duplicate

@BastiaanOlij BastiaanOlij force-pushed the BastiaanOlij:arkit31 branch from 8d3f7f7 to 7301da5 Apr 18, 2019

@BastiaanOlij

This comment has been minimized.

Copy link
Contributor Author

BastiaanOlij commented Apr 18, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.