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

Update README & BuildTiltBrush.cs for beginners #124

Merged
merged 3 commits into from
May 18, 2021

Conversation

sw0817
Copy link

@sw0817 sw0817 commented May 14, 2021

This PR is for building own app with other scenes added for beginners.

Changes

  • README - Add explanation about 'the way to build with new scenes added for beginner developers' in Building your app for Oculus Quest step.
  • BuildTiltBrush.cs - Add explanation about 'the way to build with new scenes added for beginner developers' on 1221st line.

I'm a student making a project with Open Brush as an Open Brush sample project.
While making a new project, one of the most difficult things to solve was finding a way to add my new scenes in this project.
In the case of building in the way README file is letting people know, Tilt > Build > Do Build, new scenes are never added before change BuildTiltBrush.cs but this document isn't explaining this problem, and this is very difficult to solve for beginners.
This project and document are awesome, but I think the document should be more beginner friendly.
I really like this project and I hope the sample project I will make to be able to contributes to this project. At the same time, I hope many novice developers can use this project to create many new projects with the revised document. Thank you !

Copy link
Contributor

@TimAidley TimAidley left a comment

Choose a reason for hiding this comment

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

Thank you so much for your pull request! I'm always excited when we get a new contributor!

@@ -1218,6 +1218,7 @@ public static void DoBuild(TiltBuildOptions tiltOptions)
string stamp = tiltOptions.Stamp;
SdkMode vrSdk = tiltOptions.VrSdk;
BuildOptions options = tiltOptions.UnityOptions;
// Add your new scenes in this List for your app for Oculus Quest
Copy link
Contributor

Choose a reason for hiding this comment

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

You should delete the 'for Oculus Quest' here as the same applies to PC builds as well.

Also, [optional] it might be better to rephrase this as something like:

// During the build process the Scene List in the Build Settings is ignored.
// Only the following scenes are included in the build.

... as that describes what the code does, rather than what the reader should do.

Copy link
Author

Choose a reason for hiding this comment

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

I really appreciate your reaction !!
I changed the notes with your idea.
I'm so excited with the chance to contribute this great project !!

README.md Outdated
@@ -255,6 +255,8 @@ Follow these steps to build your app for Oculus Quest:
`../Builds/OculusMobile_Release_OpenBrush_FromGui/`.
1. Run `adb install com.Icosa.OpenBrush.apk`.

**Note:** Add your new scene files' names in List '`scene`' on 1221st line in `../Assets/Editor/BuildTiltBrush.cs` before building. If you didn't, your app won't be built with those scenes even if they are put on `Scenes In Build` in `Build Settings`.
Copy link
Contributor

Choose a reason for hiding this comment

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

I would avoid mentioning specific line numbers as it's unlikely people will remember to update that if they make changes. Maybe say something like "... add to the list defined in the DoBuild method ( string[] scenes = {...} )"

@sw0817
Copy link
Author

sw0817 commented May 17, 2021

@andybak I revised DoBuild guide to your principles. It became more clear, thank you for your comment !

@mikeage mikeage merged commit 6d5bbd3 into icosa-foundation:main May 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants