It's not working anyway
Closed this issue · 9 comments
Hi, bro, ur idea is very incredible. Piper is super light TTS plugin. When I found your project, I was very surprised and happy.
But i have bad news - it's not working anyway.
When I use ur compieled piperlib.dll i get this error:
Failed to load 'Assets/ExternalAssets/UnityPiper-master/Plugins/Windows/piperlib.dll', expected x64 architecture, but was Unknown architecture. You must recompile your plugin for x64 architecture.
Okey, i download ur fork of original Piper repo. I run make file with VSC 2022 and get my instance of dll, but when i add this dll, i get another error:
Plugins: Failed to load 'Assets/ExternalAssets/UnityPiper-master/Plugins/Windows/piperlib.dll' with error '%1 is not Win32 application.
When i build ur C++ project, i see many warngings, something like this:
F:\piper-master2\src\cpp\piperlib.hpp(38,60): warning C4190: C-binding is specified for "getSynthesisConfig" but returns
The UDT type "piper::SynthesisConfig" is incompatible with the C language [F:\piper-master2\build\piperlib.vcxproj]
and
F:\piper-master2\src\cpp\piperlib.cpp(25,47): warning C4273: create_PhonemizeConfig: incompatible dll linking [F:\p
iper-master2\build\piperlib.vcxproj]
Maybe i need to build it with gcc -w64?
Also i analyse your code. Why do you use PIPER_API __declspec(dllimport) instead of PIPER_API __declspec(dllexport)? I'm really not competent enough in developing C++ plugins, but when I had to do it, I used the dllexport.
I use Windows 11, Visual Studio 2022 & VSC, Unity 2002.3.20f1
If it's realy works, i think u can compile dll's for any platforms. After this, ur repo bacome really very usefull.
Okay, I also have some recommendations for you regarding C# code. Here my competencies are quite strong.
Never do the following:
public static Task<Piper> LoadPiper(string fullEspeakDataPath)
{
if (!Directory.Exists(fullEspeakDataPath))
throw new DirectoryNotFoundException("Espeak data directory not found");
return Task.Run(() =>
{
var piperConfig = PiperLib.create_PiperConfig(fullEspeakDataPath);
try
{
PiperLib.initializePiper(piperConfig);
return Task.FromResult(new Piper(piperConfig));
}
catch
{
PiperLib.destroy_PiperConfig(piperConfig);
throw;
}
});
}
You never handle an exeption in this task. In my case i don't get any exepction, beacuse you simple throw it. Good practics in this case use Debug.LogError or another logger. Also for unmanaged code good practics use global try-catch, with listed catches:
try
{
// global unmanaged code block
}
catch(StackOverflowExecption ex)
{
// handle specific exeption
Debug.LogError(ex.Message);
}
...
catch(Execption ex)
{
// handle general exeption
Debug.LogError(ex.Message);
}
For example, my execption thrown in PiperLib.create_PiperConfig(fullEspeakDataPath); and what i see in unity? Nothing. I just press context menu button and get nothging. No result, no error. This is a bad practics.
Okey, it's work fine with compile via MinGW
Thank you for letting me know about all these issues. How did you clone the repo? This error
Failed to load 'Assets/ExternalAssets/UnityPiper-master/Plugins/Windows/piperlib.dll', expected x64 architecture, but was Unknown architecture. You must recompile your plugin for x64 architecture.
looks like LFS didn't fetch files. If you add a package via Package Manager, Unity automatically handles LFS. Maybe this didn't happen? Because why else would it complain that this is unknown architecture.
I complied this project with VS 2022 MSVC, opening the Makefile in VS.
F:\piper-master2\src\cpp\piperlib.hpp(38,60): warning C4190: C-binding is specified for "getSynthesisConfig" but returns The UDT type "piper::SynthesisConfig" is incompatible with the C language [F:\piper-master2\build\piperlib.vcxproj]
This is intended. You see, the piper was built without C bindings in mind. But it also appears, I don't need to access these structures in C#, so I basically ignored the fact they are not compatible.
About dllexport. Thank you for spotting this. Probably I've made a mistake while cleaning up #ifdef statements and didn't try compile it after.
Unfortunately, I'm my wife is giving a birth soon and I don't have time for anything except my main job. If you have a moment to check this, or build libraries for other platforms, I would appreciate any help as PRs.
Well, no, actually this function you shared is correct, the problem lies within calling function of example project. For some reason, Unity doesn't print exception messages when I return Task from context menu method. Explanation below. Fix is on it's way.
Check out this docs for exceptions inside Task.Run
Unhandled exceptions that are thrown by user code that is running inside a task are propagated back to the calling thread
When you call an async method, you just need either use await, or manually check if an exception is thown in all the cases. In my example I use await, so if exception happened inside Task.Run block that is absolutely correct and intended.
The problem why you didn't see exception, is because I added Task as return type in Run() method in my test script. To be honest, I didn't expect Unity to handle it this way. I'll push a fix in a second. Sorry for this :)
Perhaps you misunderstood me, when any call to the Run method with a task return value, you will not receive an exception, because there is no expectation in the calling method. There is nothing surprising in how this works, because regular calls to this task “as a method” will lead to a similar result. And you won't get an exception from the calling thread. After all, a call from any other asynchronous method works quite correctly, and that’s why I thought that’s what you intended, which is why I said about additional logging. Any synchronous call will result in loss of logs. And in general, I was pointing out that this example is not very user-friendly.
In general, another reason why I paid attention to this is the fact that this can happen when working with callbacks or reflection. You may say that these are specific cases, but many developers use, for example, dependency injection (DI Containers use reflection when create an instacne of class or call Construct method) or a promise system, and such an API, due to the carelessness of the developer, can lead to a similar result with the loss of logs. Therefore, it is good practice to add logging.
There's also the problem that even if it's done asynchronously, when you throw an exception, it interrupts all further execution in the root calling method, which isn't always convenient because you end up crashing all the other business logic because of one module. But this is a matter of taste. In fact, your example is quite convenient to use in another asynchronous method, but it may be necessary to call it in a synchronous method, this could be done implicitly, for example, during dependency injection, in a promise system, and then the logs will also be lost.
In any case, I wanted to say that this is not a user-friendly API. I would make one entry point, at which I would pass as parameters what I want to say: text, path to the model, link to AudioSource, and so on. And I had you choose between RunAsync and Run synchronously. In synchronous execution I can just say “hey dude do this, I won’t wait for the result, just do it”, but in asynchronous I would need to wait. And with this approach, adding logging sounds like a good idea; this could be done in the API call method itself, handle exceptions in the form of a try-catch list and, if necessary, make a Dispose unmanaged resoruces.
Thank you for letting me know about all these issues. How did you clone the repo? This error
I fork ur repo, bacause i want to add global API which I mentioned above.
To be honest, this is the first time I have come across something where LFS breaks DLL. Usually when I saw such errors, it was due to incorrect compilation of the dll file.
Yes, you are absolutely right, if you add the package through the package manager everything works perfectly.
Perhaps a little later I will try to compile for several platforms. And give users ready-made bindings for many platforms.
Thx for your answer! Your repositories Piper, LLama and Whisper helped me a lot in saving time, they are very useful and interesting.
Thank you! I appreciate any help.
Also, thank you for pointing out the missing exception logging. As you saw, I've fixed that in the example. When you open context menu of the Example component and click "Run Stream" Unity calls the method Run() and unity is supposed to handle its exception in the same way as exceptions inside Update/Awake and other are handled, now this works.
I understand your point. But I would prefer not to add such logging into the library code, as many people, including myself, prefer not to flood the console with logs from external libs. An exception simply indicates that a particular operation has failed. The client code decides how to handle it: print it in the logs, show a message, silence it, or call the same method with different arguments. Printing logs to the console is an attempt to decide the handling for the user, which is not always appropriate.
I suppose, in any case, you will have an asynchronous method similar to the Run() from the example, where you can take care of proper exception handling. The library just signals that something had failed, it doesn't decide for you, how to handle this.
I'm glad you find the plugins handy. But at this moment, the Piper is in the best state right now.
Llama package requires an update, as the recent version have changes Context structure. It works, but only with the older versions of the lib.
Whisper package is actually not finished, it works, but slow and may skip words from your speech.
Yes, as I said earlier regarding logging, it’s a matter of taste. Again, I’ll clarify that all of the above concerned only the first example, which was before your edits. Everything is fine now.
Perhaps, in general, I agree with you. In fact, Piper gave me quite a headache with various problems during the integration process. Therefore, a lot of things irritated me and in my impulse I decided to express it here.
In any case, I thank you for not ignoring some advice and responding constructively.
Regarding LLama and Whisper, I have already noticed some problems with them. For example, the version of LLama from the official repository works noticeably faster. In your example, with each new request the model is reloaded, which slows down the work even more. It is very difficult to fully check the work of LLama due to huge lags. Moreover, I caught three crashes during tests. This is no good.
So, I'm currently moving the official LLamaCpp repository with C# Bindings. Now it seems that your implementation runs on the CPU, because on my RTX 3080Ti, generating one request takes about 1-2 minutes, while in the LLamaSharp it takes only a few seconds. Yes, and there is no choice on what to launch (CPU/GPU). While in the original implementation there are separate dll files for the CPU, GPU, CUDA of different versions and so on.
Regarding Whisper, there are also C# Bindings for it, which work quite well. Perhaps as a result of my work it will be possible to solve all the problems and give the Unity user a convenient tool.
However, your repository UnityPiper is the only ready-made C# bindings that I could find. Other implementations involve communicating with the original application via sockets, which, you see, will not be very convenient for gamers.
Honestly, I am very grateful to you, because I was so lazy to do all this myself xD
Therefore, please do not abandon your repository and perhaps you will even be able to make full-fledged C# bindings. Naturally, when your wife’s birth is successful. I wish you and your family good health :)