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

InterfaceAdapter implementation for Unity #57

Merged
merged 6 commits into from
Jul 8, 2015
Merged

Conversation

JeroMiya
Copy link
Contributor

@JeroMiya JeroMiya commented Jul 4, 2015

Implements #53

Also obsoletes InterfaceCallbacks and InterfaceBase classes.

…sses now use them. Added ButtonInterface and AnalogInterface, and Requires{interfaceType} classes.
…justed RequiresAnalogInterface and the others to implement a common interface, and streamlined the API to make the IInterface<T> instance easier to access from subclasses. Updated samples to not use the obsolete classes.
@DuFF14
Copy link
Member

DuFF14 commented Jul 6, 2015

Good work, this seems to fix the scene switching bug :)
I went through each scene to make sure there weren't any errors. There were some in minigame, devscene, and UntrackedVRDisplayDemo. In minigame.unity, Left1Button and LeftAnalogTrigger throw a null reference exception when the game starts. Do you mind going through each scene and making sure the example button/analog objects are working with this? There are some other errors in these scenes that are unrelated to this work, but should be very easy to fix while you are there. For example, I think the VRDisplayUntracked prefab needs a DisplayInterface and OsvrDistortion component.

You also might want to add prefabs for button and analog objects to OSVRUnity/Prefabs.

…omponent prior to being Started by Unity. Now the getter for the IInterface properties ensure Start is called, as was done in InterfaceGameObject. Added null guards to RequireAnalogInterface and others. InterfaceGameObjectBase.Start is now idempotent, as are all the subclasses. Modified VRDisplayUntracked prefab to add a DisplayInterface and an osvr distortion component. Created new prefabs for analog and button interfaces. Modified minigame.unity and devscene.unity to use new interface types in place of InterfaceGameObjects.
@JeroMiya
Copy link
Contributor Author

JeroMiya commented Jul 7, 2015

I was able to reproduce your null reference exceptions by testing in the unity editor as opposed to just the standalone mode. They had two causes: one - minigame.unity had the wrong interface type attached to the button and analog game objects (InterfaceGameObject instead of ButtonInterface and AnalogInterface, respectively), and two I wasn't ensuring Start was called from the IInterface property getters for each interface type, as was done in InterfaceGameObject. Turns out Unity doesn't ensure Start is called prior to returning an object with GetComponent, which I wasn't expecting.

I made the requested modifications to VRDisplayUntracked prefab and added the button and analog prefabs as suggested.

I wasn't able to reproduce any other errors in these scenes. Could you describe the steps to reproduce them? I can take a second look if need be.

I've tested all the scenes (devscene, minigame, TrackerView, VRDisplayDemo, UntrackedVRDisplayDemo, OSVRDemo, and OSVRDemo2 - let me know if I missed one) in both the editor and in standalone mode with the changes in 39514ff. I'm using OSVR-Core snapshot v0.2-158-gb66f7af-build116.

@rpavlik
Copy link
Member

rpavlik commented Jul 7, 2015

@DuFF14 so should we be merging this? or do you need to add to it first?

@DuFF14
Copy link
Member

DuFF14 commented Jul 7, 2015

minigame and devscene are causing Unity to crash for me, using 4.6.5 and 4.6.1. Works for me with Unity 5.0.3f2 64bit and similar 32 bit.

Here is the editor log
Looks like a metadata issue. @JeroMiya, in Project Settings -> Editor do you have Version Control Mode set to Visible Meta Files and Asset Serialization Mode set to force text?

Other than the metadata thing in Unity 4, this looks good. After this is merged, I will update the persistent singleton branch.

@DuFF14
Copy link
Member

DuFF14 commented Jul 7, 2015

Another editor log that looks different. Looked up the "CheckConsistency: Restored Transform child parent pointer from NULL" message and found this thread about it: http://forum.unity3d.com/threads/objects-loose-info-after-import.34344/

Looks like a weird bug related to... modifying prefabs? (sorry, I asked for this) There are a couple of solutions to try here. I tried opening the scene in Unity 5, breaking prefab instances related to the prefabs we just modified, re-saving the scene and trying to open in Unity 4, but still get a crash.

edit: copying the scenes and deleting the old versions did not work. Reverting to previous commit does work. Not sure what could cause this. Perhaps the scene was saved in Unity 5, and the scene file has some properties in it that Unity 4 doesn't know what to do with?

@JeroMiya
Copy link
Contributor Author

JeroMiya commented Jul 7, 2015

Yep, that is likely it. I was editing the scenes in unity 5. I will redo the scene changes in unity 4 tonight.

@JeroMiya
Copy link
Contributor Author

JeroMiya commented Jul 8, 2015

I have confirmed that the issue was unity 5 scene/prefab incompatibility. I re-implemented the scene and prefab changes in unity 4 from the prior versions, and re-tested all scenes in that version. That fixed the console errors. I'm using unity 4.6.7.

Edit: I also confirmed that my editor settings are set as @DuFF14 described.

@DuFF14
Copy link
Member

DuFF14 commented Jul 8, 2015

I thought I'd be able to add commits here, but it appears I have to open a new pull request without push access to the forked repo. There were still some errors for me in devscene.unity, but I fixed them along with some other minor changes in the demo scenes: #58
Tested each scene and everything looks like it is working properly and can be merged.

@DuFF14 DuFF14 merged commit db94ae2 into OSVR:master Jul 8, 2015
@rpavlik
Copy link
Member

rpavlik commented Jul 9, 2015

Oh rats, forgot to have you write an entry in the changes file for anyone upgrading: what will they have to change in their existing code? Can someone do this?

@JeroMiya JeroMiya deleted the Adapters branch July 9, 2015 04:58
@JeroMiya
Copy link
Contributor Author

JeroMiya commented Jul 9, 2015

I put that change in a separate pull request: #60

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.

None yet

3 participants