📝 updated README-Unity.md code examples with new naming convention

[BRomans]

The scripts now correctly compile in Unity after the correct setup procedure is completed.

[dmedine]

Thanks for this!

Certainly the class name in the example needs to be updated to reflect the change from liblsl to LSL. However, I am not sure that I like the using static LSL.LSL directive. I would prefer to stick with using LSL and using the typename identifier when invoking the static methods, e.g.

streamInfos = LSL.resolve_stream("type", StreamType, 1, 0.0);

instead of

streamInfos = resolve_stream("type", StreamType, 1, 0.0);

This may be against Unity programming conventions (which I know absolutely nothing about), but it goes against the Microsoft C# style guide (https://docs.microsoft.com/en-us/dotnet/csharp/fundamentals/coding-style/coding-conventions):

Call static members by using the class name: ClassName.StaticMember. This practice makes code more readable by making static access clear. Don't qualify a static member defined in a base class with the name of a derived class. While that code compiles, the code readability is misleading, and the code may break in the future if you add a static member with the same name to the derived class.

MSDN's reasoning for keeping the typename identifier has more to do with inheritance safety which isn't really at stake here, but for readability's sake, I would prefer to go this way.

[BRomans]

Hi @dmedine , thanks for your feedback.
I agree it's not the most elegant solution, I was discussing it with @tstenner on Slack as an alternative to LSL.LSL.resolve_stream(..), it happens that resolve_stream( ) is a static function in a class called LSL inside the LSL namespace (hence the awkward call). If you think this is still better than using the static import, or you want to suggest an alternative, I'll change it right away.

[tstenner]

This may be against Unity programming conventions (which I know absolutely nothing about), but it goes against the Microsoft C# style guide (https://docs.microsoft.com/en-us/dotnet/csharp/fundamentals/coding-style/coding-conventions)

Good to know. In this case I'd think it's reasonable because the LSL class is just a way to circumvent C#'s inability to have static function in the namespace scope (and as such it won't be inherited from). LSL.resolve_streams makes it easier to see where resolve_streams is coming from, but it might lead to confusion because there's already a using LSL;. As it is, the only strong feeling I have about this is happiness that the examples are up to date now 😄

[dmedine]

Well, I didn't see anything in the document about naming a lone static class the same as the encapsulating namespace, but that probably isn't correct either. One thing we could do is to remove the outer LSL namespace. That would make using static LSL the correct way to do it and then inner objects and methods would not need to be prefixed. In either case, I personally find using static LSL.LSL and resolve_streams more confusing than using LSL and then LSL.resolve_streams.

[BRomans]

That sounds reasonable to me, should I change to using LSL and LSL.resolve_streams or directly change the LSL namespace's name and then update the examples accordingly?

[dmedine]

That sounds reasonable to me, should I change to using LSL and LSL.resolve_streams or directly change the LSL namespace's name and then update the examples accordingly?

I would be interested to see what @tstenner says about this. On the one hand, removing the outer namespace breaks the thing once again, but on the other hand, it isn't really necessary since there will never be another class other than LSL in the LSL namespace. The using static directive wasn't introduced until C# 6, which was a few years after the wrapper was originally composed which (I would guess) is why there is a namespace around the static class in the first place.

For now, I think we should hold off on this until we resolve the other pending PR: #17. Once that is done we can worry about changing the structure of the wrapper---and then adjust the documentation appropriately.

[tstenner]

On the one hand, removing the outer namespace breaks the thing once again, but on the other hand, it isn't really necessary since there will never be another class other than LSL in the LSL namespace.

Well, and the StreamInfo, StreamInlet and StreamOutlet classes. If the class had a better name than "LSL" (i.e. not "LSLStaticFunctionHolderClass"), I'd approve putting it in the root namespace.

[dmedine]

Well, and the StreamInfo, StreamInlet and StreamOutlet classes. If the class had a better name than "LSL" (i.e. not "LSLStaticFunctionHolderClass"), I'd approve putting it in the root namespace.

D'oh! I had it in my head that those were subclasses. Also dll.

It is a strange fate that we should suffer so much fear and doubt over so small a thing. This naming issue makes me realize once again how this whole wrapper is structured like a python library and not like a CSharp library and I believe that is why this issues is hard to deal with. My ideal liblsl-CSharp would have the following features:

1. Each class/enum should be its own file.
2. Functions like local_clock should be readonly properties and use lambdas (and should obey the style guide, i.e. have camel case names).
3. Class implementations should be internal and accessible only via Interfaces and factory methods.
4. Async methods should exist, especially for those methods that wrap asynchronous functions like pull_sample and resolve_stream.
5. Those async methods should call user-defined delegates or commands after await that the client can use to control program flow.

I would say we should either bite the bullet and actually revamp the thing or else just leave it. Maybe we could just make the change of coming up with a better name for the static class LSL, but though I for one can't think of anything that isn't confusing.

What isn't confusing is restructuring the project like a csharp project. Although that is a heartbreaking proposal, I think it is worth it. It solves this problem and a number of others that will eventually arise; and, we are just leaving money on the table by not taking advantage of async/await.

[dmedine]

And bite the bullet I did: https://github.com/Diademics-Pty-Ltd/LSL-dotnet.

I don't want to hijack this project or break anything, but it is simply too esoteric for me to handle anymore. This namespace issue was the last straw for me.

This is a much more modern way of doing things and is consistent with C# code style. I haven't added any real async methods except for one: https://github.com/Diademics-Pty-Ltd/LSL-dotnet/blob/main/src/lib/IContinuousResolver.cs#L11-L15, but this is the pattern I would like to follow: the client must supply an action to handle the result of the awaited async call, the task is then shoved onto the thread pool and executes the supplied client action. This is an incredibly convenient way to handle program flow with method calls that block or want to wait for something (e.g. pull chunk).

For now this only compiles, I haven't actually tested it yet, but I will also make a WPF based app for doing sanity checking based on this implementation and use it as an example. We need such a tool anyway and I have a lot of the bits lying around in other projects.

[tstenner]

And bite the bullet I did: https://github.com/Diademics-Pty-Ltd/LSL-dotnet.

I like the direction this is taking. 👍

In the Rust world, there's the concept of -sys-crates for wrapping C-libraries. It splits the interface into three components:

1. the native lsl library (i.e. lsl.dll, liblsl.so etc.)
2. a 1:1 wrapper for the library functions (lsl-sys) that also takes care of finding the native library, converting arguments etc.
3. a wrapper that translates common idioms into calls to the functions exported by the lsl-sys package

[dmedine]

I am still not clear on what to do abut the native code dependencies in terms of packaging liblsl-CSharp. From that long discussion we had (#2) I am no closer to knowing what to do. From what I understand the two options with nuget are either 1. ship the csharp lib with all the native code dependencies for every OS/ABI ever or 2. only ship the csharp lib and leave it to the user to find the correct native code library and add it to their project. This is the way it is done with onnxruntime (https://github.com/microsoft/onnxruntime), for example, which is in fact a Microsoft product (although it is clear from their repository that most of the developers hail very much from a Linux community). There, though, the native code is a moving target as its API is still being developed. At least LSL doesn't have this problem to deal with. Pretty much any version should be compatible with the CSharp wrapper.

I am still chipping away at what I am calling LSL-dotnet. It is a complete re-write of the interface for the wrapper. When it is done, I will bash out this whole repository with the new version in a PR. We should still merge this for now so that it is correctly documenting the current version.

[dmedine]

But this is complicated by the update Chad pushed 17 days ago. The file in this PR is actually more stylistically correct (e.g. using LSL; StreamInlet as opposed to LSL.StreamInlet).

[cboulay]

In this PR, why is the first example now using static LSL.LSL but the second example is using LSL?

[dmedine]

Sorry, not sure what you mean by 'first example' and 'second example', but I guess you mean the Unity example and the classic LSL how-to examples?

I did suggest at one point to scrap the using static LSL.LSL in favor of only using using LSL and prepending the static class calls with LSL. That keeps this README consistent with the examples. I am still in favor of that. We can merge this over your update and then make the change.

[cboulay]

Yes please. I'm not in a position to recommend syntax practices in C#. I'll continue to beat the drum that it needs to work anywhere Unity works, but other than that I have no preference.

[dmedine]

I see, there were two examples in one file.