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

Add Oculus Quest tutorial to documentation #3040

Merged
merged 1 commit into from
May 15, 2020

Conversation

skyace65
Copy link
Contributor

@skyace65 skyace65 commented Jan 5, 2020

Adds the Oculus Quest tutorial written by Bastiaan Olij to the documentation. This was taken from the Godot website post here and modified. the intro had to be changed to work with the documentation, the tutorial now uses "you" instead of "we", how some things are phrased were changed. The organization of the code was changed slightly. Aside from those changes the tutorial is the same.

@NeoSpark314
Copy link

Would be great to have a quest tutorial as part of the documentation.
What do you thing about maybe adding a line to explain that you can read godot and godot_oculus_mobile log messages at runtime with adb via $adb logcat -s godot:* GodotOVRMobile:*?

Another thing: since performance for VR (and battery life for Quest) is very critical what do you think about suggesting to always use the OvrMetricsTool (https://developer.oculus.com/blog/ovr-metrics-tool-vrapi-what-do-these-metrics-mean/) during development?

@skyace65
Copy link
Contributor Author

skyace65 commented Jan 6, 2020

@NeoSpark314 what is adb? And adding a link to the OvrMetricsTool sounds like a good idea.

@NeoSpark314
Copy link

adb is the "Android Debug Bridge". It is the tool you setup also in godot (see https://docs.godotengine.org/en/latest/getting_started/workflow/export/exporting_for_android.html) to deploy you project onto the device. So everybody will need it and have it when working with the Oculus Quest and Godot.

@NeoSpark314
Copy link

Another thing that that could be added is the current button mapping for the touch controller as part of the documentation. There is an open issue for this here GodotVR/godot_oculus_mobile#73

But note that there is currently a discussion ongoing about the ID for the grip trigger; the documentation here GodotVR/godot_oculus#4 says its mapped to 4; but actually its currently mapped to 3: GodotVR/godot_oculus_mobile#65

@skyace65 skyace65 changed the title [WIP] Add Oculus Quest tutorial to documentation Add Oculus Quest tutorial to documentation Jan 8, 2020
@skyace65
Copy link
Contributor Author

skyace65 commented Jan 8, 2020

For now I'm going to keep this PR just as the tutorial from the Godot website. I'm running into C# problems and I'm not sure if I can fix them before 3.2s release. @NeoSpark314 As for the other stuff you mentioned I do think it's worth adding that information in a update to the page, maybe as a separate page dedicated to developing for the Quest and other Oculus headsets. But for now I'm going to keep the page as is so it can be merged before 3.2 releases.

@NeoSpark314
Copy link

Ok; thanks for update. Makes sense. What would be needed to get it merged as soon as possible?

Copy link
Contributor

@Feniks-Gaming Feniks-Gaming left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some changes one big one is mixing first and 2nd person throughout a document with "we" and "you" interchanging a lot. Couple of code style suggestions

tutorials/vr/developing_for_oculus_quest.rst Outdated Show resolved Hide resolved
tutorials/vr/developing_for_oculus_quest.rst Outdated Show resolved Hide resolved
tutorials/vr/developing_for_oculus_quest.rst Outdated Show resolved Hide resolved
tutorials/vr/developing_for_oculus_quest.rst Outdated Show resolved Hide resolved
tutorials/vr/developing_for_oculus_quest.rst Outdated Show resolved Hide resolved
@skyace65
Copy link
Contributor Author

I've just fixed all the issues Feniks had with it.

Copy link
Contributor

@Feniks-Gaming Feniks-Gaming left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just 2 more comments that come to mind. Not a major one but I feel it will improve the way your example reads.


extends Spatial

var perform_runtime_config = false
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When you see perform_runtime_config you don't think this is boolean it sounds more like a function name to do action

Maybe you could rename to can_perform_runtime_config to imply in name that this can either be true or false?



func _process(_delta):
if !perform_runtime_config:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could we use if not rather than if ! I know both are correct but ! is easier to miss.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

! is more standard but I do agree also easier to miss when following a tutorial.. I still think its the more common notation though

@mhilbrunner
Copy link
Member

cc @BastiaanOlij @m4gr3d

needs to export to Android devices.

Next you need the Quest Plugin. You can download it from the Asset
Library or manually download it from `here <https://github.com/GodotVR/godot-oculus-mobile-asset>`__.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like the link is incorrectly formatted?

Copy link
Member

@Calinou Calinou May 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That seems correct to me. Double trailing underscores can be used to create unnamed references, so you won't get a Sphinx warning if the same link is used more than once in a page.

@BastiaanOlij
Copy link
Contributor

Looks pretty good to me. Agree with what was said somewhere above, lets get the basic tutorial in first, then see what we can add to it.

ADB could do with a page of its own that is referenced both from here and from the android deployment page, it really is a tool that could use a more generic reference guide for diagnosing issues.

@skyace65 skyace65 force-pushed the QuestTutorial branch 2 times, most recently from 205cfed to 5e6906d Compare May 15, 2020 13:28
@skyace65
Copy link
Contributor Author

The links are fixed along with the merge conflict.

@mhilbrunner
Copy link
Member

Thanks for keeping this PR updated with the reviews.

Seeing how long this PR is open, and that we now got a review by our resident VR wizard in, I'll merge this - the two things noted above that are still open strike me as debatable without clear consensus.

Let's merge, and do incremental improvements afterwards if neede - it's been open long enough. :)

@mhilbrunner mhilbrunner merged commit b1297a4 into godotengine:master May 15, 2020
@mhilbrunner
Copy link
Member

Thanks @skyace65 for writing the tutorial :)

@BastiaanOlij
Copy link
Contributor

@mhilbrunner feel free to ping me on twitter btw, I get such a high volume of updates from github I tent to miss things mentioning me half the time :(

@BastiaanOlij
Copy link
Contributor

Looks like some of the image links broke before the final merge

@NeoSpark314
Copy link

when I open https://docs.godotengine.org/en/latest/tutorials/vr/developing_for_oculus_quest.html all the images seem to be there; but I noticed yesterday that during a documentation rebuild there might be broken images until everything is fully rebuild again.

@mhilbrunner
Copy link
Member

I'll check what's up with this :)

@mhilbrunner
Copy link
Member

Fixed in #3550

@skyace65 skyace65 deleted the QuestTutorial branch June 3, 2020 23:09
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.

6 participants