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

Move RetroCore instantiation to load_game for better ergonomics #3

Closed
InquisitiveCoder opened this issue Oct 10, 2022 · 3 comments
Closed
Milestone

Comments

@InquisitiveCoder
Copy link
Contributor

InquisitiveCoder commented Oct 10, 2022

First off, thanks for this crate! I was able to get a toy libretro core up and running in no time, even as a Rust newbie.

That said, having to return an instance of my core in the init method was unpleasant since it meant deferring initialization of some fields. Removing init and returning the core instance in the return value of load_game in my fork of libretro-rs streamlined my core's code.

To the best of my knowledge, anything that can be done in retro_init can also be done in retro_load_game. I'm relatively new to libretro development so it's possible I overlooked something, but I couldn't find any options in libretro.h for which that wasn't the case. I also found a comment in the libretro Discord server that confirms this: "I think the idea is that retro_init should do stuff relevant for all content, and front doesn't call deinit+init if it loads new content in same core, to save some time. Not sure if any cores actually do that and benefit from it in practice. It's perfectly fine to do everything in load_game.".

Moreover, it's not possible to report errors to the frontend until retro_load_game is called since retro_init lacks a return value. It's hard for me to imagine a situation where partially initializing a core in retro_init would provide a benefit over doing all of the initialization in retro_load_game. The same user quoted above also said: "retro_init returning void is also generally considered a mistake in hindsight. the standard recommendation is set a global variable that makes retro_load_game instafail." I'd go as far as to say there probably isn't a good reason to use retro_init even in C code due to this.

While this would be a breaking change to libretro-rs, I believe it'd make the crate easier to use for most (all?) users without any loss of generality. If you want, I can open a pull request with my changes.

@ghost
Copy link

ghost commented Nov 12, 2022

Thanks for the suggestion! Sorry for the delay, I've only recently gotten back into working on this crate. Feel free to open a PR, and I'll see if it can be reconciled with the libretro documentation.

I am going to be migrating this repo over to GitLab and then mirroring it back as read-only in the near future, but I'll be sure to preserve any PRs/issues opened before that time.

@InquisitiveCoder
Copy link
Contributor Author

No worries, I'm also working on this on and off. I pulled your latest commits and opened a PR as requested.

@ghost
Copy link

ghost commented Nov 23, 2022

Closed by #4

@ghost ghost closed this as completed Nov 23, 2022
@ghost ghost added this to the 0.2.0 milestone Nov 24, 2022
This issue was closed.
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

No branches or pull requests

1 participant