deltaphc / raylib-rs

Rust bindings for raylib
Other
775 stars 136 forks source link

[0.12.0] Plan. Dynamic linking, More platforms, Less unsafe. #22

Closed Dacode45 closed 4 years ago

Dacode45 commented 5 years ago

My main goal for 0.12 is to support more platforms. This means bringing back dynamic linking for platforms we can't build off hand as well as enabling the user to select which version of raylib (2.5 or master) to include statically. I'd also like to reduce the use of unsafe in the library. Right now, the primary usage is to prevent copies with std::mem::transmute. Bringing in the zero copy crate may drastically reduce the usage of unsafe.

norcalli commented 5 years ago

The std::mem::transmute for keyboardcodes is causing me some problems right now. I can't even quite understand why transmute is being used for KeyboardCode. Assuming that enum and a number will be memory compatible is not a valid assumption, I believe. I think that transmute should be a last resort.

norcalli commented 5 years ago

In fact, I've just proven it:

        let pressed_key = rl.get_key_pressed();
        let mut d = rl.begin_drawing(&thread);
        if let Some(pressed_key) = pressed_key {
            let pressed_key: u32 = unsafe { std::mem::transmute(pressed_key) };
            println!("{}", pressed_key);
            d.draw_text(&format!("{:?}", pressed_key), 100, 12, 10, Color::BLACK);
        }

without that transmute to copy the value, I get a SIGILL error trying to format the pressed_key. unsafe has bitten me on even the smallest example so far, so I'd call this library non-idiomatic until those are removed to only necessary uses. Avoiding copy isn't the best use case.

Relevant excerpt:

Warning: There are crucial differences between an enum in the C language and Rust's C-like enumerations with this representation. An enum in C is mostly a typedef plus some named constants; in other words, an object of an enum type can hold any integer value. For example, this is often used for bitflags in C. In contrast, Rust’s C-like enumerations can only legally hold the discriminant values, everything else is undefined behaviour. Therefore, using a C-like enumeration in FFI to model a C enum is often wrong.

Dacode45 commented 5 years ago

I agree, 100%, that there's a bit too much std::mem::transmute in the library right now. This is, unfortunately, a case where a bug in the C library causes a bug in the rust wrapper. The enum is generated from the raylib.h header which should enumerate every possible value that GetKeyPressed exposes, but it doesn't. I've talked to raysan about it, but until the library gets updated I can't fix it.

Raylib is probably the simplest and most intuitive C game library out there right now. That doesn't mean there aren't some rough spots. I've probably found more bugs in raylib writing the bindings than using the library. Still, I'd rather have the bindings be unsafe enough to use rather than so safe you can only look at it. Hence the std::mem::transmute. Every one of those uses is me saying, "Hey, I'm trusting the header and ignoring everything else."

This doesn't mean I won't accept PR's to fix things like this. If you have a better method, PLEASE let me know. Here's the current implementation.

 /// Gets latest key pressed.
    #[inline]
    pub fn get_key_pressed(&self) -> Option<crate::consts::KeyboardKey> {
        let key = unsafe { ffi::GetKeyPressed() };
        if key > 0 {
            return Some(unsafe { std::mem::transmute(key as u32) });
        }
        None
    }
Dacode45 commented 4 years ago

Fixed

Dacode45 commented 4 years ago

Closing this. 0.12 was supposed to be additional support for other platforms. I added the no_build feature which lets you build raylib-rs without building raylib. From there you can link on your own.