Closed ToadKing closed 10 years ago
See #8
I don't know if SET_HW_RENDER and GET_x_DIRECTORY should be extracted - keep in mind that exported functions let the front call into the core, while env callbacks let the core call into the front. I'm pretty sure inverting GET_x_DIRECTORY will make things a bit messier, since the core will now have to copy all paths it needs. Not sure about SET_HW_RENDER, though; inverting that may make things cleaner.
One thing we don't want to do is force cores to contain dummy functions for things they don't use, which favors keeping most stuff as envs. I will also recommend merging, removal or changing of some of them.
SET_ROTATION: Does anything use this? Maybe we should remove it and tell cores to use SET_HW_RENDER and fiddle with the texture coordinates. GET_OVERSCAN: Does anything use this? And if something does, it could probably be turned into a core option. GET_CAN_DUPE: This one just returns a boolean for whether that feature is supported. Let's make support mandatory so we can remove it. SET_MESSAGE: Does anything use this, or do they all use LOG_INTERFACE? Is there any reason to use this over LOG_INTERFACE? If needed, we could add another level to LOG_INTERFACE that goes to the screen. SHUTDOWN: Does anything use this? Is there any advantage of this over exit(0);? SET_PERFORMANCE_LEVEL: Rarely used, but could be made more common if we make it mandatory. However, if we do, we need some kind of tool to tell what perf level a core should have, or everyone will just default to incorrect answers and make it pointless. Maybe it's better to keep it as an env, since they're optional. GET_SYSTEM_DIRECTORY: Keep as env, it's easier that way. Less buffers for the core to keep track of. SET_PIXEL_FORMAT: Make it mandatory; could be promoted to a function. Maybe we should make the front tell which format it prefers so the core can use that if it supports both (format_to_use=retro_get_pixel_format(RETRO_PIXEL_FORMAT_XRGB8888)). Not sure what to do if the front has no preference; maybe we could specify that the front should prefer XRGB8888 on desktop and RGB565 on mobile and console or something. (Oh, and let's nuke XRGB1555, it sucks. If people really want it, emulate it with SET_HW_RENDER.) SET_INPUT_DESCRIPTORS: As far as I remember, RetroArch does not properly support this one; it just prints it to stdout and does nothing else, so I'm not sure if anything uses it. I'd prefer keeping it (I can find some uses for it in my libretro front), but I'm not sure how much use it'll get when the primary implementation just barely cares. Either way, keep it as an env. SET_KEYBOARD_CALLBACK: Keep as env, it's rare. SET_DISK_CONTROL_INTERFACE: Keep as env, it's rare. Demote to core options if possible, I didn't check if this thing can sanely be represented as core opts. SET_HW_RENDER: I vote keep it as an env since it's optional. We don't want to force cores to contain empty functions. GET_VARIABLE, SET_VARIABLES, GET_VARIABLE_UPDATE: I want to replace those entirely. See #12. SET_SUPPORT_NO_GAME: Keep as env, it's rare. GET_LIBRETRO_PATH: Keep as env for same reasons as GET_SYSTEM_DIRECTORY. SET_AUDIO_CALLBACK: Keep as env, it's rare. SET_FRAME_TIME_CALLBACK: Keep as env, it's rare. GET_RUMBLE_INTERFACE: Keep as env, it's rare. GET_INPUT_DEVICE_CAPABILITIES: Keep as env, it's rare. GET_SENSOR_INTERFACE: Keep as env, it's rare. GET_CAMERA_INTERFACE: Keep as env, it's rare. GET_LOG_INTERFACE: Could probably be promoted to a function, it's common enough. But on the other hand, not everything uses it, and it may be easier to let the core ask for it; I'm not sure. I vote keep the current state so we get one thing less to change. GET_PERF_INTERFACE: Keep as env, it's rare. GET_LOCATION_INTERFACE: Keep as env, it's rare. GET_CONTENT_DIRECTORY: Keep as env for same reasons as GET_SYSTEM_DIRECTORY. GET_SAVE_DIRECTORY: Keep as env for same reasons as GET_SYSTEM_DIRECTORY. SET_SYSTEM_AV_INFO: Keep as env, it's rare. SET_PROC_ADDRESS_CALLBACK: Does anything use this? Since libretro.h:1069-1070 says "libretro API extension functions: (None here so far).", I suspect no, so I vote remove. (Keep retro_proc_address_t, though; SET_HW_RENDER uses it.) SET_SUBSYSTEM_INFO: Keep as env, it's rare. SET_CONTROLLER_INFO: Keep as env, it's rare. SET_MEMORY_MAPS: Keep as env, it's rare. It seems easier to implement as an env, too; doing it that way allows the core to tear that structure down once it's been sent to the front. (On a semi-related note, someone should probably hook it up to something in RetroArch, as well as finding a somewhat sensible representation for bankswitched data. But we can do that once the ABI break is done, it's marked experimental anyways.) SET_GEOMETRY: Keep as env, it's rare. GET_USERNAME: Keep as env, it's rare. GET_LANGUAGE: Keep as env, it's rare.
Any disagreements?
Having no retro_get_proc_address() is an oversight in the main API. Because of that, you cannot easily query core things directly (except doing set_environ and grab callbacks indirectly, but it's kinda awkward). If there's a libretro v2, more environ callbacks should become direct function calls where get_proc_address would be nicer.
SET_ROTATION: FBA/Arcade. GET_OVERSCAN: Kinda obsolete. Used by SNES cores at least. GET_CAN_DUPE: Useless. Just mandate support. I don't think any core actually checks this, but all frontends support it. SET_MESSAGE: FFmpeg core uses it. SHUTDOWN: FFmpeg uses it. Useful when the core naturally ends (e.g. movie ends). SET_PERFORMANCE_LEVEL: I think this feature is mostly useless. SET_PIXEL_FORMAT: Should be something the frontend queries, really. SET_INPUT_DESCRIPTORS: Not used yet, iirc. SET_HW_RENDER: Should be integrated better with libretro core API. GET_VARIABLE, SET_VARIABLES, GET_VARIABLE_UPDATE: Could be cleaner. SET_SUPPORT_NO_GAME: Should be a simple query to core. SET_PROC_ADDRESS_CALLBACK: Not yet, but should be more used.
Most of the interfaces should really be done with retro_register_interfaces() kind of functions instead of environment calls (kinda like Wayland).
Anyways, really depends how much you're willing to change the libretro v1 API for maintenance concerns, developer churn, etc.
There are more things we should consider just deferring to interfaces. E.g. serialization doesn't make sense for all cores, and neither does cheats. There is a lot of ugly boilerplate we have to implement for each core.
I'd rather not add a retro_get_proc_address; I'd rather have bool retro_reverse_environment(unsigned cmd, void *data);
(except with a better name). Using strings in one part of libretro and integers in another, for doing pretty much the same thing, sounds like a good way to create a mess.
(HW_RENDER can continue using strings; glGetProcAddress uses strings, and the front is probably just a thin wrapper for that. No point doing anything weird over there.)
ROTATION: Is HW_RENDER is a sufficient replacement? (I'm not entirely sure why the core can't just render in the right direction directly, but I'll assume there's some kind of hard-to-change legacy cruft going on.) OVERSCAN: Is a core option a sufficient replacement? CAN_DUPE: Gambatte checks it and aborts if it's false. But yeah, make mandatory, it's easy. MESSAGE: Is LOG_INTERFACE (possibly with a fifth level) a sufficient replacement? SHUTDOWN: FFmpeg sure does a lot of stuff the other cores don't... If we want to keep it, I want to know whether "shutdown" means exit(0), retro_unload_game(), or something else. (And whether "movie end" counts as "from a menu item or similar"; if we're going to let FFmpeg use it, let's at least refine the comments a bit.) PIXEL_FORMAT: That's exactly what I said. Does my proposal seem satisfactory? PERFORMANCE_LEVEL: Squarepusher seems to have some kind of plans for it. Let's keep it, he'll be sad if we don't. INPUT_DESCRIPTORS: Let's keep it for now. HW_RENDER: What exactly do you propose? Remember that most cores don't use this, and we don't want to force the ones that don't to implement empty boilerplate functions. VARIABLE/etc: As I said, that's issue #12. NO_GAME: Should, yes. How about putting this in retro_system_info::valid_extensions? Set it to NULL if you don't want a game. PROC_ADDRESS: Covered in the first paragraph.
Serialization makes sense for most if not all cores. Yes, some of them neglect to implement it (likely due to being based on some external project which doesn't know what a savestate is), but I'd consider that a core bug and therefore not a valid argument in either direction.
The current cheat interface is, for all practical purposes, worthless; the code string format is undefined, the core can't tell whether the code is valid, both front and core need to keep track of the same cheat list, and various other flaws. It looks like leftovers from bsnes; I see no value whatsoever in keeping it. The memory map env I made is intended to grant cheat support with far less effort per core, by putting as much code as possible in the frontend where it benefits all cores.
Interfaces may be a good idea, but I'll need to see a concrete example before I can judge that. The env calls have worked fine thus far, I see no strong reasons to change them.
Right now there are nearly 40 environment variable callbacks. We need to decide which ones of these should be promoted to regular API features and which should stay. Obvious stuff like hardware rendering and path callbacks are easy shoe-ins for promotion, but all of them should be gone through and looked at.