Closed Hokage3211 closed 4 years ago
Scorr
commented
4 years ago Hey thanks for the PR. It seems like you also included the code from #12 in this but not the RGB8888Job.cs causing a compile error. Instead of including the job I'd prefer if you could exclude it entirely because it's a seperate PR + doesn't entirely work yet.
Other than that, I've tested it and seems good to merge. Happy to see save states work :)
Hokage3211
commented
4 years ago yeah it started as a pull request for just saves, and I'm not quite experienced with unity pull requests to a second repository, so wasn't sure what would and wouldn't get included, seems even commits afterwords are being included. I did also find a fix for the genesis issue, using code from https://github.com/3DArcade/3DArcade/tree/master/Assets/Libretro/Scripts/Wrapper. I'll probably push that here as well. I will note my method makes it so it doesn't load the game immediately, because swapping cores and game text on the script module became a chore, so I just had it be press page-up to choose the game file then core file, and I will probably have to include the file browser asset i used, since unity for some reason doesn't have an end-user-application ready file choosing system. I also fixed the sound even better, so it now waits for good data to be read in from the stream, as I noticed one rom of mine didn't send any data for a while which made the sound delay come back.
Does this sound like a good idea, or would you prefer I try to strip it down to the bare parts that fixed what needed to be done? I've been kindof absorbing a ton of code about this, because I have no idea where anyone is getting how to work with these cores, so I'm implementing everything as properly as I can find in hopes it fixes random problems.
Hokage3211
commented
4 years ago New commits fix Genesis rendering problem and improves sound quality for all cores I've tested to work. I've only been able to work on this by using several example code bases found all over the web to try and add more compatibility, and improve stability. But I've no idea how any of you all know how to code for this, I can only go off examples I find that happen to have new stuff, that I try and see if they work or add good stuff.
mGBA is unstable, will sometimes crash and sometimes work with no code changes, don't know why, and PS1 and n64 emulation still doesn't work, though I got gearsystem to not crash by adding the set_subsystem command, it just displays black with a few pixels now.
Scorr
commented
4 years ago My first pull requests were the same way haha. In the future, what you should do is make a new branch for each seperate PR/feature. Any commits you make on that branch will show up in the PR like what happened here. Try to keep the scope small, one PR per feature. This is because 1. else it becomes a lot to digest and 2. I can't approve one commit but not the other, meaning if one of your changes does not fit then all the others in same PR would be blocked also.
Anyway I'm gonna try to sum up your changes and reply to them seperately. tl;dr is at the bottom in case it's too long.
Sound fix To be honest this kind of worries me, because it's not entirely clear to me why these changes fix the issues. In the first commit one of the consequences is that the sound is delayed in the first few seconds of starting, which is not good. Second commit fixes this, but again I'm not entirely sure how it all works. It seems to still have some minor issues, maybe related to the trailing numbers you mentioned in the code (mentioning potential issues in comments is very good, :+1: for that). But it was a lot worse before so I welcome the improvements.
Saving/loading feature Nice, no criticism here.
Pressing page-up to load I agree that the current way of loading rom/core is a chore. And although page-up is a decent solution for an example scene, I'd like something more visible and intuitive. I'll probably work on this soon, thanks for bringing it to my attention.
Genesis fix I haven't tested a Genesis core in the latest commit, but it still has the same issue where you're referencing the RGB8888Job from the other PR causing a compile error. As mentioned in previous comment, I'd rather you don't include code from other PR's.
Extra commands and options This is pretty neat, but I'm planning on refactoring the wrapper so I'd hold off on any significant changes here.
Regarding using other code bases, that's how this project started as well. Sharing code (with attribution where applicable) is a great and powerful thing. But I don't want RetroUnity to be a copy, so I'd appreciate if you don't commit code from other codebases. I believe in the ideal case there would be seperate libretro Unity/C# implementations existing alongside eachother, with different pros and cons and unique architectures.
That's part of the reason why a rework has been on my todo list , but I lost interest and didn't think other people would find this repo. Now that I know some people are interested, I'll probably try to get a major rework going in the next few weeks.
Of course if you need these features for your own project, I absolutely recommend you keep using them in your local repo/project.
tl;dr: I won't merge the current PR as-is, but if you can seperate your sound fix and saving/loading code, I'd like to merge that as a seperate PR. It might be a pain to seperate them, so if that's too much I'd recommend you keep using your local changes and wait until after the rework.
Anyway, I truly appreciate all the help so far. I never expected other people to use this project and it warms my heart that people are even wanting to contribute to it.
Hokage3211
commented
4 years ago I must have forgotten to include my changed version of the job file, sorry about that, I'll just re-do this pull request. Otherwise, I don't understand what you mean by "don't want it to be a copy" and "seperate libretro implementations existing" when there has never actually been a single implementation to do it all (have at least compatibility with all cores, like actual retroarch does), making it unnecessarily hard for any one person to find out how to do this on their own, so my goal is to have /at least/ one full-blown example for people to look for, rather than making them go through the same pain that we all have to. Were there an existing all-in-one example, then I'd say your comment is valid.
Fixing sound delay, and adding save files using RAM saving as well as save states, compatible with retroarch's actual files, for cross-loading. RAM saves and states are created in the same directory as the rom file is, for convenience, but they don't have to be, the functions take a full path to create the file, so it can be wherever.