chroma-sdk / Colore

A powerful C# library for Razer Chroma's SDK
https://chroma-sdk.github.io/Colore/
MIT License
146 stars 30 forks source link

Compatibility with Unity #157

Closed njbmartin closed 8 years ago

njbmartin commented 8 years ago

As discussed previously, some of the color conversions have rendered the colore library incompatible.

Are we agreed that setting up a separate build specifically for Unity is the way forward?

Sharparam commented 8 years ago

Unless we want to completely remove conversion between different format and only support the regular, and encourage users to use the ColorMine library you mentioned?

Or is the usage of the implicit cast operators on Color too important to get rid of?

cguckes commented 8 years ago

Sorry for chiming in uninvited, but would it be possible to use the uint representation internally and extract the whole color issue into a different project? That way one could write different access modules, one for WPF colors, one for unity colors, etc. I'd happy to join in, and help, if you want me to.

Basically one would need an interface, seperating the Color representation from all the internal hardware related stuff...

Sharparam commented 8 years ago

Problem with that is we can't have the nice cast operators we currently have (so you can pass in a System.Color to any Colore function accepting a Color struct and it will convert automatically).

Moving Color stuff to a separate project is a neat idea but then we may as well leave the conversion up to the users with a third party library like ColorMine since we can't use the cast operators which is the main drive for having it baked into Color.

cguckes commented 8 years ago

It doesn't necessarily have to be another project, as long as there is a dll somewhere in there not depending on wpf or log4j or any other non .net-2.0 subset compatible assembly. In my fork, I replaced all occurences of Color and was done in like 2 hours, so it's not a lot of work to replace it with fixed uint values. The test cases wouldn't necessarily be affected, since the class would still exist, it just wouldn't be used that deep in the code.

Sharparam commented 8 years ago

Replacing Color with some new arbitrary object is not an option as it adds too much complexity and maintainability issues, not to mention the humongous refactoring problems it would introduce when something has to be changed.

Both Colore and log4net use .NET 3.5. Unity using a modified version of .NET 2.0 is a bit of an issue because of that sometimes but we can't sensibly drop support down to a .NET version that is over 10 years old as we probably use features that were only added in 3.5.

Other than PresentationCore not being available to Unity, I haven't heard any issues about log4net from the people currently using it in Unity, is there more information on this?

cguckes commented 8 years ago

Well color does not need to be replaced, I was merely suggesting, using the uint that represents the color inside the Color object throughout the original library and adding a "compatibility-layer" (being the current Color object for all .NET-3.5 users) so the interface of the library doesn't change. Then switching to a unity compatible version would only necessitate another class that takes unity colors instead of the current color object. This could look something like the attached UML diagram. This would allow you to maintain a common core for both versions without much work.

Regarding the log4net issue, I have no idea, I just removed any dlls I didn't really need in order to find out what worked in Unity and what didn't. I got the stuff to work without it and just filed it as solved, that's why I mentioned it. Might be nonsensical.

colore-layered.pdf .

njbmartin commented 8 years ago

The only problem unity has with Colore is with any references to wpfcolor as discussed a while ago.

Sent from my iPhone

On 19 Apr 2016, at 13:41, Christopher Guckes notifications@github.com wrote:

Well color does not need to be replaced, I was merely suggesting, using the uint that represents the color inside the Color object throughout the original library and adding a "compatibility-layer" (being the current Color object for all .NET-3.5 users) so the interface of the library doesn't change. Then switching to a unity compatible version would only necessitate another class that takes unity colors instead of the current color object. This could look something like the attached UML diagram. This would allow you to maintain a common core for both versions without much work.

Regarding the log4net issue, I have no idea, I just removed any dlls I didn't really need in order to find out what worked in Unity and what didn't. I got the stuff to work without it and just filed it as solved, that's why I mentioned it. Might be nonsensical.

colore-layered.pdf .

— You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub

Sharparam commented 8 years ago

@njbmartin: So you can confirm log4net works fine with Unity?

cguckes commented 8 years ago

If I remeber correctly, System.Drawing caused problems as well.

Just checked: Drawing is not in compat-level 2.0 subset. I know you don't necessarily care about that, but since it's the standard setting I imagine, a lot of people still use it...

njbmartin commented 8 years ago

So, if developers are able to change the API level in Unity from the default, then only WPFColor prevents Colore from loading.

As @cguckes is suggesting, if it's set to the default... there are other issues.

Do we then cater to people using (forced to use) the default setting? In @cguckes case, it's a mod for Kerbal Space Program which is using the default API level.

Sharparam commented 8 years ago

forced to use

Are people unable to change the API level to something sensible like 3.5?

For games I suppose it makes sense, but we also can't drop Colore down to 2.0 which would be required in addition to remove other stuff, which means we have to drop everything that doesn't exist in 2.0. If we are using LINQ anywhere that is going out the window.

njbmartin commented 8 years ago

In the case of mods, developers are stuck with whatever the game developers set it to.

cguckes commented 8 years ago

The fork I made works with .net2.0 (the reduced mono subset). So I don't see any problem there... As I said, all I did was replace the Color class...

njbmartin commented 8 years ago

@Sharparam You suggested a while ago having a 3rd build target specifically for Unity. I suggest we go ahead with that, and use the work @cguckes has done as the base for what changes need to be made.

njbmartin commented 8 years ago

@cguckes I've had a look at your fork. I don't recommend it a suitable solution for supporting Unity.

<Reference Include="UnityEngine">
      <HintPath>..\..\..\Games\SteamApps\steamapps\common\Kerbal Space Program\KSP_x64_Data\Managed\UnityEngine.dll</HintPath>
 </Reference>

This file path won't be the same for everyone, and is of course very specific to Kerbal.

cguckes commented 8 years ago

Oh right, thats a remnant of me not knowing much about c#. Is there a way to locate the dll automatically? The Color class is the only one taken out of that dll, maybe there is another way to get it... Something like %UnityInstallationFolder%/System/UnityEngine.dll.

njbmartin commented 8 years ago

No it wouldn't be possible. In the case of mods, you'd have to target the specific version of the unity dll that the game is using too.

In any case, Colore should not be referencing Unity at all, and any conversion from Unity's color should be done within the game / mod itself, not the Colore library. Within Unity or the mod, you can have just done: var color= new Corale.Colore.Core.Color(_unity._Color.R, _unity._Color.G, _unity._Color.B); after modifying the Color class to remove references to System.Drawing and WPFColor.

Sharparam commented 8 years ago

@njbmartin The third build target was when the only change needed was to drop WPF color support. If we have to support .NET 2.0 as well it would be thousands of pre-processor #ifs all over the place in addition to supporting an entirely different .NET level.

If Unity support requires .NET 2.0 then we can't just remove stuff to make Colore "compatible". If Colore still targets .NET 3.5 then it is by definition incompatible with .NET 2.0, that it happens to work with it with certain removals is just coincidence and nothing that can be relied upon or supported by us.

.NET is backwards-compatible only. A 2.0 application (Unity game) cannot reference a library targeted against a higher .NET version (3.5) and expect it to work (as it would require forwards compatibility).

njbmartin commented 8 years ago

Yes I agree to that point, however we should still continue to support Unity as much as we can considering the value it has.

Unity is still very confused in terms of what framework they support.

Unity currently supports .NET 2.0/3.5 era functionality for both the C# compiler and the class libraries.

But it's still referred as 2.0 within Unity. The subset is a portable version of it, which further removes a number of classes to help keep the filesize down.

njbmartin commented 8 years ago

So, what's the plan with this?

njbmartin commented 8 years ago

Can we implement this into the next major release?

brandonscott commented 8 years ago

Discussing implementation in Slack. For the record the options are:

Sharparam commented 8 years ago

I have implemented the second option that @brandonscott noted above in the feature/separate-integrations branch. We can use that branch to see what usage would be like with the project separation method.

brandonscott commented 8 years ago

@njbmartin Have you checked in with any Unity devs on the above?

njbmartin commented 8 years ago

Sorry no i haven't. I will check it works with Unity later.

njbmartin commented 8 years ago

The idea is to make Colore available as an asset in the Unity Asset Store, making it super easy for developers to include in their Unity projects.

https://www.assetstore.unity3d.com/

Xinhuan commented 8 years ago

I have a local clone of the repo. In it, I stripped System.Windows.Media.Color, System.Drawing.Color removed, and log4net (and linked it against UnityEngine.Debug.Log instead - which I would strip out for Release).

For work licensing reasons, I only have a licensed copy of Visual Studio 2012, so I also downgraded any C# 6 language features (all of it is due to lambda getters, null-conditional operator, and nameof() expressions) so it will compile in the older compiler in .NET Framework 3.5 which Unity requires. The final result is then usable in Unity.

If anything, I would highly recommend not using any C# 6 features in order to support older versions of Visual Studio.

njbmartin commented 8 years ago

Thanks for your input @Xinhuan. Much appreciated.

We definitely need to ensure Unity support, and it would be unwise to not provide support to Unity developers.

However, VS2012 is a little too old to support, and I would advise upgrading your licenses to at least 2013, if not 2015.

Sharparam commented 8 years ago

I think we'll be moving to extract WPF and WinForms functionality to two separate projects which can then be added via NuGet (so people that need WPF stuff can just add the package Colore.WPF or some-such in the future).

As for C# 6.0 features, usage of them will not affect the binary releases which are compatible with anything that supports .NET 3.5 and higher. There's also the free community version of Visual Studio which can be used to do develop work on Colore and I think there was some kind of "backport"-esque thing to bring Roslyn compilers to older versions of Visual Studio? Not sure about the last one and if it can be added without modifying the project.

log4net has earlier been reported as working with Unity so I don't see a reason to stripping that out and losing the logging functionality which would also gimp support as there won't be any data to analyze. log4net can be configured to output its logging data to any supported pipelines or custom ones so if integration with Unity's logging system is what is important that can be solved in the application itself.

Xinhuan commented 8 years ago

Well, for the moment, I needed to manually recompile the dll in order to remove the non-Unity supported Color stuff, which necessitated removing C# 6.0 features.

The free community version of VS cannot be used for commercial purposes. Requiring developers to buy newer versions of VS, or jump through hoops to bring newer compiler support to older versions of VS is not what I call "providing support to Unity developers", the experience should be as hassle free as possible, and be supported as widely as possible. Requiring the latest compiler is the opposite, which is very narrow support.

The final release binary should also have all logging stripped out of being compiled. It is currently yet another hassle to have to download and include the log4net binary separately into a Unity game project, which is why I think it should not have logging included, or provide alternative builds without their use.

brandonscott commented 8 years ago

Thanks for your comments. Are you integrating Razer Chroma into a commercial product or game? If so, may I ask which?

If you'd prefer not to say publicly, feel free to email me: brandon@brandonscott.co.uk

We can then discuss your requirements.

njbmartin commented 8 years ago

As of right now, Colore is not supported in Unity projects outright. The aim is to bring back support for Unity, and that focus is on supporting Mono (.Net 3.5).

We previously had it compiling just fine for Unity, until we were made aware by a Unity dev that the new WPF color conversions prevented it from being loaded into Unity. The log4net does not cause any issues, and it is already included within the release binaries of Colore.

As for the VS2012 and C# 6.0, this should not affect you if you are using our release binaries (once we've merged and compiled the proposed changes for Unity support).

I'd suggest waiting until we have completed this task, and then you can simply download the release binaries without needing to compile them yourself and thus, no requirement for VS2012.

We'd love to see if that works for you. If it's not fit for your solution, then by all means continue with your own fork.

Xinhuan commented 8 years ago

I am integrating Razer Chroma into this game (Edit: Which is set for release in the next 2+ months, hence I cannot afford to wait for Unity support):

https://www.kickstarter.com/projects/13978330/masquerada-songs-and-shadows/posts/1586656

Most of the integration is already done, so there should be little need for me to update or recompile Colore (though I may need to add support for CustomKey otherwise keybinds might not highlight properly on different keyboard layouts, not 100% sure yet - I am also uncertain if using CustomKey would break backwards compatibility with users who do not update their Chroma and device drivers). I am just providing feedback on my integration experiences so far to help future Unity developers.

njbmartin commented 8 years ago

Your feedback is greatly appreciated. Sorry it's been a little difficult getting Colore working with Unity and I can imagine the tight deadline too.

Sharparam commented 8 years ago

After the changes in #177 and #185, Colore is now compatible with all Unity versions. Closing this issue as resolved.