search

BananaHemic / Mumble-Unity

Performant Mumble Client For Unity3D
MIT License
77 stars 29 forks source link

Use of MumbleSharp #55

Closed bdovaz closed 6 months ago

bdovaz commented 6 months ago

@BananaHemic why isn't MumbleSharp used as a dependency? https://www.nuget.org/packages/MumbleSharp

I see that at the very least we can share the Mumble.cs class which is the one with the protobuff-net data model (3k lines of code).

I see that the loading of the native dlls is done differently and the use of it in Unity is somewhat different as far as OPUS is concerned. Part of this problem could be solved by making a PR in MumbleSharp to expose a custom callback for loading native assemblies.

But for everything else couldn't we make use of MumbleSharp and only implement what is strictly necessary? That way it would be easier to maintain the code and only focus on what is unique to Unity.

EDIT: apart warnings like:

[WARNING]: This server has the ChannelListener feature enabled but your client version does not support it. This means that users might be listening to what you are saying in your channel without you noticing! You can solve this issue by upgrading to Mumble 1.4.0 or newer.

This is because the Unity code was implemented when the server was around 1.2.x. And as I say, not using MumbleSharp makes it difficult to stay on the latest version of the client.

BananaHemic commented 6 months ago

I'm not really a fan of taking on MumbleSharp as a dependency. If I was starting this project anew I might go that route, but most of the behavior that is in MumbleSharp has already been separately implemented. Personally, I think it would be easier to add the ChannelListener feature, although you are correct that this will require more maintenance down the line as additional features are added to Mumble

bdovaz commented 6 months ago

@BananaHemic And you find it wrong to take this dependency to at least gradually share more and more code?

I can create a PR that:

  1. Removes the Mumble.cs class and use MumbleSharp's (protobuff code).
  2. Use the MumbleSharp enums we can share and remove them from Mumble-Unity
  3. Use the IMumbleProtocol pattern for events which is much cleaner.

I can help that little by little we use more MumbleSharp code and that the code in Unity is the essential. It will be hard to get there but little by little and PR to PR we can do it.

BananaHemic commented 6 months ago

Normally, I would agree with you that it's better to take MumbleSharp as a dependency, but I think that's not the right choice for this Unity-focused repo. The reason being that with game development it's important to keep code size small and it's useful to be able to read through every line of code without having to open up a separate dll.

This is just my opinion, however. You're more than welcome to make a fork with that dependency. I'd help any way that I can.