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

Expose remaining environment API #16

Open
ghost opened this issue Nov 28, 2022 · 6 comments
Open

Expose remaining environment API #16

ghost opened this issue Nov 28, 2022 · 6 comments
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed
Milestone

Comments

@ghost
Copy link

ghost commented Nov 28, 2022

Many of the calls for get_raw, set_raw, and cmd_raw aren't modeled properly. Effort should be focused on getting them exposed in an idiomatic way so that more of the unsafe API surface is reduced. This doesn't need to be as restrictive as #2 for the moment, but laying the groundwork for it would be nice.

@ghost ghost added enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed labels Nov 28, 2022
@ghost ghost added this to the 0.2.0 milestone Nov 28, 2022
@InquisitiveCoder
Copy link
Contributor

Should we bother implementing the experimental commands? Since they could change or be removed at any time, any safe Rust interface we implement would be unstable and could become unsafe.

@ghost
Copy link
Author

ghost commented Nov 30, 2022

Should we bother implementing the experimental commands? Since they could change or be removed at any time, any safe Rust interface we implement would be unstable and could become unsafe.

I'd be okay with putting these behind a feature flag, with the advisory that these are unstable and subject to change. Those that are willing to take the plunge do so at their own risk, and we would officially not consider it public API from a semver perspective. Perhaps experimental?

@InquisitiveCoder
Copy link
Contributor

Makes sense. I'll implement those last, then.

@ghost
Copy link
Author

ghost commented Dec 2, 2022

I've thought about this more, and the idea that this crate could be used to create potentially invalid cores I think means we should hold off implementing environment calls until they are stabilized. The *_raw functions still exist if someone wants to do something exotic, but I don't want to officially support them (By adding our own APIs) until they're stable.

@InquisitiveCoder
Copy link
Contributor

InquisitiveCoder commented Dec 3, 2022

After thinking about it some more, I don't think the experimental options are actually changed in practice. From looking at the environment commands in libretro.h, what seems to happen instead is that the constant for the command is removed, the frontends stop supporting it (i.e. they return false) and a new version is introduced. For instance, RETRO_ENVIRONMENT_GET_VARIABLE and SET_VARIABLES used to be keys 4 and 5, but those two keys have since been removed and updated versions were assigned to 15 and 16. I think by "change" they really mean "replace".

A properly written core ought to check whether the frontend supports any environment command (stable or otherwise) so there shouldn't be any trouble when an experimental command is deprecated/replaced. I think we could support them with a feature flag to opt into it like you originally suggested.

@ghost
Copy link
Author

ghost commented Dec 3, 2022

Well, that's a nasty bit of API design on their end. I suppose if that's how business is done in this domain, having them behind an unstable feature flag should suffice.

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

1 participant