Use idiomatic .Net naming conventions and other best practices

[JeroMiya]

Use this as a reference for naming conventions: https://msdn.microsoft.com/en-us/library/ms229002(v=vs.110).aspx

Here are a few issues I found:

• OSVR namespace should be Osvr.
• InterfaceBase has two public readonly properties interfaceGameObject and osvrInterface. They should be named InterfaceGameObject and OsvrInterface.
• InterfaceCallbacks has a public string field path. It should be a property and pascal cased.
• InterfaceCallbacks callback delegates types should be declared outside the class.
• ClientKit has a public string AppId. If possible, this should be a property (unless the unity editor requires it to be a field?)
• ClientKit properties instance and context, readonly property should be pascal cased.
• OSVRUnityBuild class should be named OsvrUnityBuild.
• OSVRUnityBuild.build should be OsvrUnityBuild.Build
• ColorChanger.materialID should be a property and named MaterialID
• ColorItem fields name and color should be properties and named Name and Color.
• ColorManager fields colors, changeDuration, changeDelayRandomMax, delay, and duration should be properties and pascal cased.
• HandleButtonPress.handleButton should be HandleButton.
• SampleAnalog.callback should be Callback.
• SampleButtonScript.handleButton should be HandleButton.
• DLLSearchPathFixer should be named DllSearchPathFixer.
• DLLSearchPathFixer.fix should be Fix.
• InterfaceGameObject.path should be a property and pascal cased.
• InterfaceGameObject properties osvrInterface and interfaceGameObject should be pascal cased.
• OrientationInterface.path should be a property and pascal cased.
• OrientationInterface.callback should be pascal cased.
• PoseInterface.callback should be Callback.
• PositionInterface.path should be a property and pascal cased.
• PositionInterface.callback should be Callback.
• VREye fields eye and cachedTransform should be a property and pascal cased.
• VRHead fields viewMode, stereoAmount, maxStereo should be properties and pascal cased, unless Unity requires them to be fields?
• ClientContext consts/statics OSVR_CORE_DLL, OSVR_RETURN_SUCCESS, OSVR_RETURN_FAILURE should be OsvrCoreDll, OsvrReturnSuccess, and OsvrReturnFailure. Snake casing is not generally done in .Net code. Especially not all-caps snake casing. Everything should be either pascal cased or camel cased depending on the context.
• ClientContext DllImport methods should be pascal cased. Use DllImport.EntryPoint to map these to their camel cased native methods.
• ClientContext methods shutdown, update, getInterface, and getStringParameter should be pascal cased.
• I'm not if the unity runtime has the same property getter/setter inlining as the regular .net framework, but if it does, then the client report structs (PositionReport, OrientationReport, etc...) should be using pascal cased properties, not public fields. To test: you'll notice about a 34% drop in performance with properties vs. fields if Unity isn't inlining them.
• Interface.OSVR_CORE_DLL should be OsvrCoreDll.
• Interface DllImport methods should be pascal cased. Use DllImport.EntryPoint to map them to their camel cased native methods.
• Interface.registerCallback and overloads should be pascal cased.
• Pose3, Quaternion, TimeValue, and Vec3 fields should be properties and pascal cased (assuming Unity inlines properties)

[rpavlik]

Awesome, thanks for sharing this! I'm just waiting to hear back on how pull requests work before the license change.

[rpavlik]

So some of these conflict with apparent Unity coding practice (they don't appear to pascal-case as much - and some of their stuff requires specifically-named methods, so I'd be careful), but those that are just in Managed-OSVR sound good to me.

I had no idea properties would be faster than fields, seems backwards but you're more of the expert here by far.

The code that is directly over the C layer basically resembles the C++ wrapper, which is why there is all-caps snake case for C defines, etc. No problem with changing those. Was actually thinking a bit ago when poking at a different part of the .NET code that that basically the raw p/invoke stuff should almost be moved into an internal class or something, instead of being public static methods.

The one catch I noticed in the list was that Pose3, Quaternion, TimeValue, and Vec3 are structs for p/invoke, so I do not know how much they can be changed without breaking that. In any case, they're intended for use only to get the data into your engine's native format. My concern is that there isn't automated testing of this binding at the moment, so I am hesitant to touch things lest a regression sneak in.

[JeroMiya]

Clarification: properties should be exactly as fast as fields if the version of Mono used by Unity is inlining them properly (.net 4.5 and CoreCLR inline them for sure), not faster than fields. That being said, I'm ok with using public fields for structs in general - I'm not confident that Unity's version of Mono is properly inlining property getter/setters given that it's still .net 2.0, and you bring up a good point about structs used to marshall data for P/Invoke.

Note, this isn't without precedent. For example, MonoGame is an open source port of Microsoft's XNA framework, which was originally released for XBox 360. MonoGame structs all have public fields, not properties, because the original runtime on the xbox didn't support inlining.

Also, since writing this issue, I noticed that Unity does indeed require public fields if you want something to be visible from the editor, which is what I suspected. There were some workarounds, but if that's the prevailing convention for Unity projects I say just go with the flow. Same goes with camelCase naming conventions for methods, though I think this is more to do with Unity sharing some script engine code with the JavaScript bindings. That being said, I would limit this to the Unity specific code and follow the .net naming conventions in Managed-OSVR as you suggest.

[rpavlik]

OK, sounds good. I've duplicated out the Managed-OSVR component into its own repo and done a few of your suggested changes (primarily the assembly name changes - though I looked up in that document and it looks like it's OK to have it be OSVR in all caps since that's the product/company name effectively), so most of this can probably be moved to that project. Sadly I don't know a way to move GitHub issues to a different project.