koute / libretro-backend

Libretro API bindings for Rust
Other
46 stars 16 forks source link

Roadmap Discussion #2

Open mehcode opened 7 years ago

mehcode commented 7 years ago

@koute Really neat start you have here. I was considering doing something like this. I'm wondering how much you're committed to this and where you see this library going.

I see a lot that can be cleaned up, refactored, etc. One big one is keeping closer to libretro terminology.

fn system_info(&self) -> SystemInfo {
  SystemInfo::new("Pinky")
    .version("0.0")
    .supports_extensions(&["nes", "nes2"])
    .requires_path(false)
}

Any thoughts? I can do things bit by bit in pull requests.

koute commented 7 years ago

Use https://github.com/rust-lang-nursery/lazy-static.rs for the instance mutable static

Yep!

to_cstring( name ) can be replaced with CString::new

Yep! I have no idea why I made the to_cstring function.

instead of supports_roms_with_extension("nes") perhaps supports_extensions(&["nes"])

I like the name with rom in it (or something similar, if there) as that leaves no room for confusion (you're going to call this only in a single place in the whole emulator so there is no real point in having it short), and as far as the method taking an array - I've made it to take only a single argument as a) emulators usually don't support a huge number of extensions and this is going to be called only once in the whole emulator so the extra repetition isn't a problem, and b) to make it possible to specify the supported extensions only in a single way. With a function taking a single extension the only way to use it for multiple extensions is to call it multiple times, while with a function taking multiple extensions it might not be entirely obvious what will happen if it will be called multiple times - will the specified extensions will be added or replaced? But I guess maybe it's not entirely obvious that the function is supposed to be called multiple times?

Nevertheless I would be fine with the variant that takes as array, provided there would be an assert in there that would ensure it won't be called multiple times.

Rather than ::on_initialize() perhaps ::info() or ::system_info() implemented like:

Hmm.. info()... Yep! I like it. (Although I prefer current requires_path_when_loading_roms name as I've explained already, and I think having version in the constructor is OK as a) that's a very basic piece of information that every emulator has to pass, b) having its own helper method doesn't net us anything since it's always going to be called either like this "1.0.0" where it's obvious that the version or like this MY_EMULATOR_VERSION where the fact that it's a version number is in the constant's name so it's also obvious.)

libretro-backend => retro-core — Generally I don't see the C convention of "lib-" being carried over into the rust name.

In this case it's not about the C convention of attaching lib to libraries, but simply because libretro authors call their project libretro instead of simply retro. (Notice how, for example, SDL authors don't call their library libsdl but simply SDL, unlike libretro guys.)

Backend => Core is a big one I'd think

I'm a little ambivalent about this but I guess it makes sense.

I'm wondering how much you're committed to this and where you see this library going.

Unfortunately I didn't have too much time to work on this for the past couple of months, but I'd definitely like to support more functionality; as to where I'd like this to go - I care much more about keeping it idiomatic Rust and having it convenient to use rather than keeping it 100% inline with the libretro API. Easy to use, obvious what it does without reading the docs API, sane defaults and as much error checking as possible enforced by the type system.

I do welcome any contributions you might have!