-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Draft: Fix #746 (CLI) Auto-detect Core when -L isn't present #15458
base: master
Are you sure you want to change the base?
Conversation
I think this is welcome functionality and indeed picking some core behind the scenes is better than failing with no core at all. For multiple cores, I think the ideal behavior would be to launch into the menu at the point of the core picker dialog, just as you would if you just 'load content'. |
I attempted to get @hizzlekizzle 's idea working, but can't figure out how to control the menu programatically. I tried something along the lines of
Could I get a hint for what to do? |
Made some progress, with the caveat that it involves moving menu initialization earlier. :( |
Looking at the menu code, it's ok to initialize the menu multiple times, even early. @LibretroAdmin could you comment on direction here? The other option is figuring out some deferral method that waits until after the menu is actually initialized (some sort of hook in the main menu?). There's a case that doesn't work as well as I'd like - downloading:
I need to do another pass at C89 compat, but will do so after I know I'm not barking up the wrong tree. |
Fixed (2) and (3) - there was a bug in how the stack was popped - now downloading a core, waiting, and backing out does the right thing (it would be extra neat if the list refreshed itself, but I'm not super worried about it) |
....ok, now when you go to download the core, you're only presented with cores associated with the content. The way I implemented the clear on cancel seems wrong, please lemme know if there's a nicer way. What I really want to do is associate a cancel option with the window, not every item on the window, even though I get why the extra flexibility would be nice. Please lemme know the "proper" way of doing this, I feel like I took a sledgehammer to the menu system. |
being able to download a core from there is a great idea, and having it only show eligible cores is even better. :) good stuff! |
I had been thinking about trying my hand at this, but I hadn't started yet. I have written a shell script that looks at .info files for file extensions and then prompts you to enter a number which core you want if more than one core supports a file extension, and was thinking about doing the same in RetroArch when it is built with --disable-menu. |
Hi, quite a few CI tasks are failing. Can you check it out and fix it? |
Yes, of course. I'm out of town for four days, but I will circle back to this next week. |
Let us know when you're ready to look into the matter. |
Just sharing that this has been on my wishlist FOREVER. Thanks a lot for picking it up. I tried building locally, and got the following...
As a side-note to #15458 (comment) , make sure to Update Assets. |
I am going to try to work on this tomorrow. Got back from vacation, got slammed with work, and now MAGWEST is upon me. :) |
@LibretroAdmin fixed the build errors |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
{ | ||
core_info_list_t* core_info_list = NULL; | ||
|
||
/* TODO: error printing */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Error printing?
runloop_set_current_core_type(CORE_TYPE_DUMMY, false); | ||
#ifdef HAVE_MENU | ||
/* this is a hack, we should be deferring the core list until after we have loaded menu */ | ||
/* TODO: ask how to unhackify it */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm, good point. I haven't tested this without the menu being available yet. Do you have any ideas on how we could "do it right"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would think automatically picking the first in the list would be pretty much the only way to go.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In other projects with similar issues I've printed out a list of options and asked the user to enter a number.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, it seems we have to first fix an RA compilation issue when using ./configure --disable-menu
Compilation log
CC tasks/task_manual_content_scan.c
tasks/task_manual_content_scan.c: In function 'task_manual_content_scan_handler':
tasks/task_manual_content_scan.c:215:16: warning: implicit declaration of function 'runloop_msg_queue_push' [-Wimplicit-function-declaration]
215 | runloop_msg_queue_push(
| ^~~~~~~~~~~~~~~~~~~~~~
CC tasks/task_core_backup.c
CC libretro-common/encodings/encoding_utf.c
tasks/task_manual_content_scan.c:218:28: error: 'MESSAGE_QUEUE_ICON_DEFAULT' undeclared (first use in this function)
218 | NULL, MESSAGE_QUEUE_ICON_DEFAULT, MESSAGE_QUEUE_CATEGORY_INFO);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~
tasks/task_manual_content_scan.c:218:28: note: each undeclared identifier is reported only once for each function it appears in
tasks/task_manual_content_scan.c:218:56: error: 'MESSAGE_QUEUE_CATEGORY_INFO' undeclared (first use in this function)
218 | NULL, MESSAGE_QUEUE_ICON_DEFAULT, MESSAGE_QUEUE_CATEGORY_INFO);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~
tasks/task_manual_content_scan.c: In function 'task_push_manual_content_scan':
tasks/task_manual_content_scan.c:576:19: error: 'MESSAGE_QUEUE_ICON_DEFAULT' undeclared (first use in this function)
576 | NULL, MESSAGE_QUEUE_ICON_DEFAULT, MESSAGE_QUEUE_CATEGORY_INFO);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~
CC libretro-common/encodings/encoding_crc32.c
tasks/task_manual_content_scan.c:576:47: error: 'MESSAGE_QUEUE_CATEGORY_INFO' undeclared (first use in this function)
576 | NULL, MESSAGE_QUEUE_ICON_DEFAULT, MESSAGE_QUEUE_CATEGORY_INFO);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~
make: *** [Makefile:209: obj-unix/release/tasks/task_manual_content_scan.o] Error 1
make: *** Waiting for unfinished jobs....
`
Edit
Compilation issue is fixed with this PR, last error is linking issue.
Linking issue log
`
/usr/bin/ld: obj-unix/release/retroarch.o: in function `retroarch_main_init':
retroarch.c:(.text+0x8b2d): undefined reference to `handle_dbscan_finished'
collect2: error: ld returned 1 exit status
make: *** [Makefile:204: retroarch] Error 1
`
Edit 2
Linking issue is fixed with f34481c
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @zig-for will it be possible to rebase so that you can get the two fixes for the option --disable-menu
?
Thank you.
/*This isn't needed, is it?*/ | ||
/*command_event(CMD_EVENT_LOAD_CORE_PERSIST, NULL);*/ | ||
core_info_get_list(&core_info_list); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔
Co-authored-by: Rob Loach <[email protected]>
Co-authored-by: Rob Loach <[email protected]>
Co-authored-by: Rob Loach <[email protected]>
Co-authored-by: Rob Loach <[email protected]>
Co-authored-by: Rob Loach <[email protected]>
Co-authored-by: Rob Loach <[email protected]>
Not for a few weeks. Slammed with work.Sent from my iPhoneOn Sep 13, 2023, at 5:19 AM, gouchi ***@***.***> wrote:
@gouchi commented on this pull request.
In runloop.c:
@@ -3615,9 +3680,51 @@ bool runloop_init_libretro_symbols(
if (string_is_empty(path))
{
- RARCH_ERR("[Core]: Frontend is built for dynamic libretro cores, but "
+ const char* content_path = path_get(RARCH_PATH_CONTENT);
+ if (!string_is_empty(content_path))
+ {
+ bool core_loaded = auto_load_core(content_path);
+ if (!core_loaded) {
+ struct menu_state* menu_state = menu_state_get_ptr();
+
+ CORE_SYMBOLS(SYMBOL_DUMMY);
+ runloop_set_current_core_type(CORE_TYPE_DUMMY, false);
+#ifdef HAVE_MENU
+ /* this is a hack, we should be deferring the core list until after we have loaded menu */
+ /* TODO: ask how to unhackify it */
Hi @zig-for will it be possible to rebase so that you can get the two fixes for the option --disable-menu ?
Thank you.
—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: ***@***.***>
|
Thanks, brought in the latest from |
Guidelines
Description
This allows Retroarch to attempt to pick the proper core when loading. Windows already does this on drag and drop.
The PR as is doesn't handle missing cores, nor does it delegate to the frontend if there's multiple possible cores. Happy to put work towards that if there's a desire for this to go in. The main desire is to try to push for a "good" solution even if no perfect solution exists (I'd rather a happy path of being able to easily set file extensions associated with RA to do the right thing, then give up on every case just because some cases are hard to handle).
To repeat - as is, the PR is not ready, just looking for direction on both implementation and probability of acceptance later
EDIT: Additional changes:
Current bugs:
Related Issues
#746
#9275
https://forums.libretro.com/t/autodetect-core-when-opening-a-rom-file/1863
#1704
#8557
Reviewers
[If possible @mention all the people that should review your pull request]