Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Xunkar's AI service rework updated #15640

Merged
merged 20 commits into from
Oct 24, 2023

Conversation

cpod12
Copy link
Contributor

@cpod12 cpod12 commented Aug 23, 2023

Original PR by Xunkar here #15175.
This one should be up to date and able to automatically merge.

@cpod12 cpod12 changed the title Xunkar's AI service rework Xunkar's AI service rework updated Aug 23, 2023
@xunkar
Copy link
Contributor

xunkar commented Aug 23, 2023

Well you've been busy while I was sleeping, good work :D

@cpod12
Copy link
Contributor Author

cpod12 commented Aug 23, 2023

Thank you! I'm glad I could help. :) Definitely take a look at it. I wasn't sure how to test it out

@hizzlekizzle
Copy link
Contributor

@LibretroAdmin Dunno if you want to try and sneak this in before the next stable or push the stable out first so this can be tested in nightlies.

@LibretroAdmin
Copy link
Contributor

Couple of things:

@cpod12
Copy link
Contributor Author

cpod12 commented Aug 23, 2023

Ok it should be good now @LibretroAdmin. Lmk if there's anything wrong or else that needs to be fixed

accessibility.h Outdated Show resolved Hide resolved
retroarch.cfg Outdated Show resolved Hide resolved
retroarch.cfg Outdated Show resolved Hide resolved
retroarch.cfg Outdated Show resolved Hide resolved
retroarch.cfg Outdated Show resolved Hide resolved
tasks/task_translation.c Outdated Show resolved Hide resolved
@LibretroAdmin
Copy link
Contributor

OK, so now we're in the final stretch of this PR being approved, I've asked @BarryJRowe to review this as well since he created the AI code.

@BarryJRowe
Copy link
Contributor

As a test, I tried this with FF1 on a nestopia core with the ztranslate.net service, and it only reported no text found with just the word error reported in the log. Master branch works fine, so it's not a configuration issue.

Last time there was an issue with supporting the GL driver, so I tried changing the driver to vulkan, or glcore, but it just gave the same error. Requests going to the service look to be properly formatted, and the service returns valid results, but they are not being parsed.

@xunkar
Copy link
Contributor

xunkar commented Aug 31, 2023

Considering it used to work, I think it's related to the recent changes made by @cpod12 specifically in translation_grab_frame the removal of the call to video_driver_cached_frame_get. The new code doesn't seem to populate the frame cache data, but I understand the basecode changed and this function was removed?

@BarryJRowe
Copy link
Contributor

Any luck @cpod12 ? The main thing I'm concerned about is compatibility with the existing services.

@cpod12
Copy link
Contributor Author

cpod12 commented Sep 7, 2023

Sorry @BarryJRowe. I was out of town for the last week. I'll look into all that soon. Do you have advice for running cores with builds? I don't know how to do that.

@cpod12
Copy link
Contributor Author

cpod12 commented Sep 7, 2023

Nvm don't answer that. It shows it in the build guide lol.

@cpod12
Copy link
Contributor Author

cpod12 commented Sep 9, 2023

So I'm having trouble getting content to run. After I load a core and get to content that I try to run it says no core found even with the core selected. Does anyone know what could be causing this?

@cpod12
Copy link
Contributor Author

cpod12 commented Sep 9, 2023

Nvm again lol. I figured it out. It should be working ok now @BarryJRowe. Lmk if there's anything wrong or if you're having any issues

@BarryJRowe
Copy link
Contributor

I'll try to check it out either over the current weekend, or early next week.

@BarryJRowe
Copy link
Contributor

I've done some testing and after updating the ztranslate service it looks good to me. The only suggestion I have would be to merge the latest of master into the branch and test before merging.

On a side note however, there should be some documentation available on what's expected for the subs output from the remote api server, as well as any additional information about the automatic functionality with an example implementation.

@cpod12
Copy link
Contributor Author

cpod12 commented Sep 29, 2023

Ok, that should be fine. @xunkar would you be able to add that documentation? I'm not familiar with it enough to write that. Especially the second part about the automatic functionality.

@xunkar
Copy link
Contributor

xunkar commented Sep 29, 2023

accessibility.h has all the documentation necessary, has it not?

@BarryJRowe
Copy link
Contributor

accessibility.h has all the documentation necessary, has it not?

Authors who want to develop their own API should not have to dig through the C code as a first step (to get specifics that's understandable, but not just to start out with trying to understand a basic request).

I would recommend making changes to this page:

https://docs.libretro.com/guides/ai-service/

Github link for what file to change to do so: https://github.com/libretro/docs/blob/master/docs/guides/ai-service.md

As an example, the ztranslate service api documentation was given here when it was first released: https://ztranslate.net/docs/service though it has added some more features over time. Enough information should be provided to get something working, such as the input format for the body, and the other valid input queries. Things that you did not change (like the key pressing stuff) don't have to be added, but things like the new subs type should be explained. Specifics of the implementation can be point the reader to the retroarch code.

@xunkar
Copy link
Contributor

xunkar commented Oct 12, 2023

OK, I did my best. But I'm not a native english speaker, so it may not be a bad idea to proof-read my work. Also, I have no idea how to merge the change (it's over at https://github.com/xunkar/docs) with this particular PR.

@hizzlekizzle
Copy link
Contributor

You can just do a PR from your docs fork to the docs repo and then link that PR here as a "merge this one too".

@xunkar
Copy link
Contributor

xunkar commented Oct 13, 2023

Oh ok thanks. However, I have no idea why, but somehow the commit is nowhere to be found now? I edited the file and all… and I don't know it reverted back, and I have no local copy. This… might take a while.

@hizzlekizzle
Copy link
Contributor

@xunkar
Copy link
Contributor

xunkar commented Oct 14, 2023

You're a life saver, thanks :D

@LibretroAdmin
Copy link
Contributor

I'm going to assume @BarryJRowe tested this.

@LibretroAdmin LibretroAdmin merged commit 274d47f into libretro:master Oct 24, 2023
22 checks passed
JoeOsborn pushed a commit to JoeOsborn/RetroArch that referenced this pull request Nov 2, 2023
* AI service rework

* File missing

* Fixed C89 build

* Fixed usage of inline for C89 build

* Fixed an overlay unloading bug

Made sure to unload the overlay on release and when the server returns empty values in automatic modes.

* Fixed forward decl (c89)

* Fixed OpenGL texture loading

Moved image display to the main thread for now

* Changed some formatting slightly

* Fixed struct variable order and put brackets on newlines

* Moved pointer, fixed retroarch.cfg, and replaced strlcat with strlcpy

* Fixed catenation issue

* Fixed a few other catenation issues

* Fixed one more concatenation  issue

* Fixed concatenation issue

* Fixed a few other concatenation issues

* Fixed one more concatenation  issue

* potential fix for parsing issue

---------

Co-authored-by: Xunkar <329857+xunkar@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants