godot-rust / gdnative

Rust bindings for Godot 3
https://godot-rust.github.io
MIT License
3.61k stars 210 forks source link

Option<Image> parameter becomes null after adding layer to Texture3D #257

Closed maxeonyx closed 4 years ago

maxeonyx commented 4 years ago

Hi -

I am making a small program to create procedural 3D (volumetric) textures.

Currently I am very confused. I have just encountered an issue with the bindings where a parameter was being incorrectly set to null. The API in question takes an Option<Image> parameter.

I have fixed my problem by editing the bindings files and changing the parameter type from Option<Image> to just Image, and replacing the if let Some(arg) = image { ... } else { ptr::null() } line.

Obviously modifying the binding code is not ideal. I was wondering if I am doing something wrong, or whether this is a bug.

Below is my before and after versions of the files.

tex_3d.rs (my code) (before)

...
    for iz in 0..SIZE_Z {
        let mut layer = get_image();
        tex.set_layer_data(Some(layer), iz as i64);
    }
...

tex_3d.rs (my code) (after)

...
    for iz in 0..SIZE_Z {
        let layer = get_image();
        tex.set_layer_data(layer, iz as i64);
    }
...

bindings_types.rs (before)

...
    #[inline]
    pub fn set_layer_data(&mut self, image: Option<Image>, layer: i64) -> () {
        unsafe { TextureLayered_set_layer_data(self.this, image, layer) }
    }
...

bindings_types.rs (after)

...
    #[inline]
    pub fn set_layer_data(&mut self, image: Image, layer: i64) -> () {
        unsafe { TextureLayered_set_layer_data(self.this, image, layer) }
    }
...

bindings_methods.rs (before)

...
#[doc(hidden)]
pub unsafe fn TextureLayered_set_layer_data(obj_ptr: *mut sys::godot_object, image: Option<Image>, layer: i64) -> () {
    let gd_api = get_api();

    let method_bind: *mut sys::godot_method_bind = TextureLayeredMethodTable::get(gd_api).set_layer_data;

    let mut argument_buffer : [*const libc::c_void; 2] = [
        if let Some(arg) = image { arg.this as *const _ as *const _ } else { ptr::null() },
        (&layer) as *const _ as *const _,

    ];

    let ret_ptr = ptr::null_mut();

    (gd_api.godot_method_bind_ptrcall)(method_bind, obj_ptr, argument_buffer.as_mut_ptr() as *mut _, ret_ptr as *mut _);
}
...

bindings_methods.rs (after)

...
#[doc(hidden)]
pub unsafe fn TextureLayered_set_layer_data(obj_ptr: *mut sys::godot_object, image: Image, layer: i64) -> () {
    let gd_api = get_api();

    let method_bind: *mut sys::godot_method_bind = TextureLayeredMethodTable::get(gd_api).set_layer_data;

    let mut argument_buffer : [*const libc::c_void; 2] = [
        image.this as *const _ as *const _,
        (&layer) as *const _ as *const _,

    ];

    let ret_ptr = ptr::null_mut();

    (gd_api.godot_method_bind_ptrcall)(method_bind, obj_ptr, argument_buffer.as_mut_ptr() as *mut _, ret_ptr as *mut _);
}
...
maxeonyx commented 4 years ago

Here I observe the correct behaviour in the first but not the second:

// this is the line after I applied my "fix"
image.this as *const _ as *const _,
// this is the above line with only a dbg!() added
dbg!(image).this as *const _ as *const _,
ghost commented 4 years ago

Image is reference-counted. Could it be that the pattern match (or dbg invocation) consumed the value, causing the image to be dropped? Pointers don't have lifetimes, which could explain why it passed compilation. No, that was a bad theory. Actually I think it might explain the first case:

    let mut argument_buffer : [*const libc::c_void; 2] = [
        if let Some(arg) = image {
            // Image is not Copy, so arg is moved here
            arg.this as *const _ as *const _
            // arg is dropped when it goes out of scope. When its destructor runs, the `Ref<Image>`
            // that `arg.this` is pointing to gets set to null, since it would be the only reference
        } else { ptr::null() },
        (&layer) as *const _ as *const _,
    ];

    // here argument_buffer[0] is not null, but the `Ref` it is pointing to is containing a null reference
    // drop(image); // If this theory is true, then this won't compile, since `image` is already dropped

If argument_buffer[0] is not null but the Godot Ref is, then this might explain the original bug. I'm not sure about the dbg! one though. dbg! could be explained because it returns the value moved in as a temporary one, for which no let binding was created. That value can be dropped by the compiler immediately after it's used.


If this theory is correct, then if let Some(arg) = &image should fix the bug.

maxeonyx commented 4 years ago

Yeah it's the godot Ref that's null. Godot is warning me that condition !img.is_valid is true, which means that the Ref is null. Your hypothesis is quite subtle and probably right - I will give it a try tonight.

maxeonyx commented 4 years ago

@toasteater You're right - adding the borrow in the destructuring fixes the issue.

if let Some(arg) = &image { arg.this as *const _ as *const _ } else { ptr::null() },