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

Problems with destroy ergonomics when using it from Drop #2452

Open
LaylBongers opened this issue Oct 27, 2018 · 10 comments
Open

Problems with destroy ergonomics when using it from Drop #2452

LaylBongers opened this issue Oct 27, 2018 · 10 comments

Comments

@LaylBongers
Copy link

LaylBongers commented Oct 27, 2018

Currently, when destroying any backend structure explicitly, such as a buffer, you need to pass it by value.

device.destroy_buffer(vertex_buffer);

These semantics make sense in principle, but in practice it makes it hard to write simple wrappers that RAII wrap around these types.

impl Drop for Mesh {
    fn drop(&self) {
        let device = &self.common.device;
        unsafe { device.destroy_buffer(self.vertex_buffer); } // < can't move out of reference error
    }
}

A solution is to wrap all these types in Option, but this quickly becomes very cumbersome just for this one specific use. Since these functions are already unsafe, why not just allow them to take &mut?

@zakarumych
Copy link

zakarumych commented Oct 27, 2018

I use ManuallyDrop wrapper

@LaylBongers
Copy link
Author

How can ManuallyDrop be used to solve this problem?

@kvark
Copy link
Member

kvark commented Oct 27, 2018

These semantics make sense in principle, but in practice it makes it hard to write simple wrappers that RAII wrap around these types.

I have many thoughts on the issue, but no solution:

  1. Did you consider not using RAII for graphics stuff?
  2. I'd really like to see drop accepting things by value. This has been my pet peeve with Rust since the beginning. I don't care how special should the compiler glue be, moving out of life just makes so much more sense as borrowing.
  3. https://github.com/gfx-rs/wgpu will have droppable objects, but it's a slightly higher level API.

@LaylBongers
Copy link
Author

The more specific use case I'm running into is, for example, putting a rendering System in a specs World. The rendering system itself owns some graphics resources, but there's no way to entirely get back ownership to the system, or even access it again.

There may be some better ways to solve this issue, perhaps this is enough of a common use case that it would make sense to have an example on it to both inform gfx and specs' design?

@kvark
Copy link
Member

kvark commented Oct 27, 2018 via email

@LaylBongers
Copy link
Author

LaylBongers commented Oct 27, 2018

Personally, I really enjoy working at a low level like gfx-hal provides, while still having the benefits of easier cross-platform-native porting. This specific API issue is the main one I ran into when using gfx-hal in this way. Is there any problem with changing the API to use &mut instead? To compare it to other libraries, when working with Ash you would pass a Copy handle to the destroy method, and as a result it doesn't have this issue.

Alternatively, a workaround would be useful.

@zakarumych
Copy link

@LaylConway ManuallyDrop works in similar way as Option does in this situation. But it doesn't consume additional memory.
Basically in Drop implementation you just std::ptr::read its content and destroy.

@kvark
Copy link
Member

kvark commented Oct 29, 2018

@LaylConway @omni-viral please have a look at #2457 RFC

@shepmaster
Copy link

This question also came up on Stack Overflow. One answer there shows usage of Option as well as ManuallyDrop.

@Lokathor
Copy link
Contributor

Lokathor commented Jan 9, 2019

Using ManuallyDrop is probably the most ergonomic answer here. The Deref and DerefMut impl means and you don't have to think about stepping through the wrapping layer during all the rest of the code.

Using the "read and destroy" technique that omni-viral alluded to would look something like this:

use core::ptr::read;
self.device.destroy_render_pass(ManuallyDrop::into_inner(read(&mut self.render_pass)));

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants