Skip to content

Oculus Camera App Prototype#11

Merged
natezzz merged 12 commits intomasterfrom
camera-app
Dec 3, 2015
Merged

Oculus Camera App Prototype#11
natezzz merged 12 commits intomasterfrom
camera-app

Conversation

@natezzz
Copy link
Copy Markdown
Contributor

@natezzz natezzz commented Nov 30, 2015

Added basic functionality for camera app. (Take screenshots by pressing F key)

@takuti takuti added this to the Camera App Improvement milestone Nov 30, 2015
@takuti takuti added the Oculus label Nov 30, 2015
@tomoasleep
Copy link
Copy Markdown
Member

Are there left tasks which you should do before we start to implement touch interaction features?
If there are not, we review your code in order to merge 👍

@natezzz
Copy link
Copy Markdown
Contributor Author

natezzz commented Nov 30, 2015

@tomoasleep Nope, except that I haven't tested the code because I don't have a Leap Motion at hand. If with Leap it can display screenshot correctly, then it's okay to merge.

@tomoasleep
Copy link
Copy Markdown
Member

OK, we check and review your implementation 👍

@Sushisushi-sandesu/leap-team というわけでよろ 🙏

@tomoasleep tomoasleep changed the title [WIP] Oculus Camera App Prototype Oculus Camera App Prototype Nov 30, 2015
Comment thread Assets/Scripts/ScreenShotScript.cs Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I want you to make this method private if it is not a callback method or is not called from other classes.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Cannot. they are overriden methods which are to be called by Unity engine.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Callback methods such as Start and Update are defined in superclasses, but this method TakeScreenshot seems not to be defined in superclasses.

http://docs.unity3d.com/ScriptReference/MonoBehaviour.html

@ganmacs
Copy link
Copy Markdown
Contributor

ganmacs commented Dec 2, 2015

👀 👍

@tomoasleep
Copy link
Copy Markdown
Member

I think the default code format of MonoDevelop is Mono,
but it is ok to space between method names and parentheses.

@natezzz
Copy link
Copy Markdown
Contributor Author

natezzz commented Dec 2, 2015

@tomoasleep Yes. The default is Mono style.
However I used "visual studio" style for this code.

@natezzz
Copy link
Copy Markdown
Contributor Author

natezzz commented Dec 2, 2015

Ah. Clicked "close" by accident.

@tomoasleep
Copy link
Copy Markdown
Member

I see :bowtie:

@tomoasleep
Copy link
Copy Markdown
Member

I think your code style is different from Visual Studio code style in if statement and method call.
https://msdn.microsoft.com/en-us/library/ff926074.aspx

@natezzz
Copy link
Copy Markdown
Contributor Author

natezzz commented Dec 2, 2015

@tomoasleep Yes. The Mono formatter didn't format the code completely according to the Microsoft convention. I don't know why.
Maybe we should use mono style instead?

@tomoasleep
Copy link
Copy Markdown
Member

Looks good to me (OK to merge if another one says OK)

@tomoasleep
Copy link
Copy Markdown
Member

Forget to check if it moves... I check it now.

@takuti
Copy link
Copy Markdown
Contributor

takuti commented Dec 3, 2015

It works (also w/ LeapMotion cam)!!

ok

All errors are caused because,

screen shot 2015-12-03 at 9 14 35 pm

all of them are not loaded in an initial cloned repository.

@natezzz
Copy link
Copy Markdown
Contributor Author

natezzz commented Dec 3, 2015

ああああああああああ、泣きそう @takuti

@natezzz
Copy link
Copy Markdown
Contributor Author

natezzz commented Dec 3, 2015

@takuti But what caused it not to be loaded though?

But it's awesome that it worked!!

@takuti
Copy link
Copy Markdown
Contributor

takuti commented Dec 3, 2015

🎉 🎉 🎉 🎉 🌞

@takuti
Copy link
Copy Markdown
Contributor

takuti commented Dec 3, 2015

@natezzz I don't know...are any important files describing connection between the canvas and related files in .gitignore...?

@takuti
Copy link
Copy Markdown
Contributor

takuti commented Dec 3, 2015

After solving the initial loading problem, we can merge this pull request 😨

@natezzz
Copy link
Copy Markdown
Contributor Author

natezzz commented Dec 3, 2015

@takuti Yes!

However I think the .meta files in this commit are indeed broken.

@takuti
Copy link
Copy Markdown
Contributor

takuti commented Dec 3, 2015

@natezzz Now my sushi project repo works correctly, and current output of git status is

On branch camera-app
Your branch is up-to-date with 'origin/camera-app'.
Changes not staged for commit:
  (use "git add/rm <file>..." to update what will be committed)
  (use "git checkout -- <file>..." to discard changes in working directory)

        modified:   Assets/_Scenes/CameraScene.unity
        deleted:    Assets/_Scenes/MainScene.unity.meta
        modified:   ProjectSettings/AudioManager.asset
        modified:   ProjectSettings/DynamicsManager.asset
        modified:   ProjectSettings/EditorBuildSettings.asset
        modified:   ProjectSettings/EditorSettings.asset
        modified:   ProjectSettings/GraphicsSettings.asset
        modified:   ProjectSettings/InputManager.asset
        modified:   ProjectSettings/NavMeshAreas.asset
        modified:   ProjectSettings/NetworkManager.asset
        modified:   ProjectSettings/Physics2DSettings.asset
        modified:   ProjectSettings/ProjectSettings.asset
        modified:   ProjectSettings/QualitySettings.asset
        modified:   ProjectSettings/TagManager.asset
        modified:   ProjectSettings/TimeManager.asset

Untracked files:
  (use "git add <file>..." to include in what will be committed)

        Assets/Scripts/ScreenShotScript.cs.meta
        Assets/_Scenes/CameraScene.unity.meta

Hence, the reasons will be in them.

@natezzz
Copy link
Copy Markdown
Contributor Author

natezzz commented Dec 3, 2015 via email

@natezzz
Copy link
Copy Markdown
Contributor Author

natezzz commented Dec 3, 2015

@takuti please wait.

@natezzz
Copy link
Copy Markdown
Contributor Author

natezzz commented Dec 3, 2015

@takuti I'll push another commit

@takuti
Copy link
Copy Markdown
Contributor

takuti commented Dec 3, 2015

@natezzz yes plz

@tomoasleep
Copy link
Copy Markdown
Member

3 'Fix .meta problem' ...

@natezzz
Copy link
Copy Markdown
Contributor Author

natezzz commented Dec 3, 2015

@tomoasleep Sorry. Kinda messed up a little during commit ...

@natezzz
Copy link
Copy Markdown
Contributor Author

natezzz commented Dec 3, 2015

@tomoasleep @takuti I just "git clone" and checked. It should be working fine now. Please review.

@takuti
Copy link
Copy Markdown
Contributor

takuti commented Dec 3, 2015

I have checked on my local, and it works well by just importing Leap Motion core assets. It was able to capture a screenshot as I posted before! 👍

LGTM

@natezzz
Copy link
Copy Markdown
Contributor Author

natezzz commented Dec 3, 2015

@takuti Nice. Now we need another reviewer and we are ready to merge. 👍
@Sushisushi-sandesu/all Someone please review if convenient 🙏

@tomoasleep
Copy link
Copy Markdown
Member

@natezzz good job 🎉
LGTM

@natezzz
Copy link
Copy Markdown
Contributor Author

natezzz commented Dec 3, 2015

Thank you! @tomoasleep @takuti

natezzz added a commit that referenced this pull request Dec 3, 2015
@natezzz natezzz merged commit 0cf6601 into master Dec 3, 2015
@natezzz natezzz removed the Oculus label Dec 3, 2015
@natezzz natezzz deleted the camera-app branch December 3, 2015 13:27
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