Lifetime issue

[beltegeuse]

trafficstars

Hi Will,

I tried to integrate your library to my renderer, but I have some issue with lifetime definition. Indeed, I want to "encapsulate" some embree_rs objects inside a single struct to make my code modular (code example below).

pub struct EmbreeAcceleration<'device, 'scene> {
    pub device: embree_rs::Device,
    pub scene: embree_rs::Scene<'device>,
    pub rtscene: embree_rs::CommittedScene<'scene>,
    pub trimesh: Vec<embree_rs::TriangleMesh<'device>>
}
impl<'device, 'scene> EmbreeAcceleration<'device, 'scene> {
    pub fn new(meshes: &Vec<geometry::Mesh>) -> Self {
        let device = embree_rs::Device::new();
        let scene = embree_rs::Scene::new(&device);
        let mut trimesh = vec![];
        for m in meshes {
            // TODO: Create trimesh and register it
            unimplemented!()
        }
        let rtscene = scene.commit();
        EmbreeAcceleration {
            device,
            scene,
            rtscene,
            trimesh,
        }
    }
}

Note that the code above does not compile. Have you tried to do a similar thing with this library in one of your projects? If you know a solution, it will be much appreciated :smile:

Cheers, Adrien

[Twinklebear]

Hm it will be tricky with some of the lifetimes here, since the scene borrows the device, and the committed scene borrows the scene. So it's a bunch of self-referencing objects in the same struct.

For the triangle geometry it's also similar, but for that I'd recommend not keeping the trimesh vector on your application side and instead storing the IDs of the meshes if you need to look them up again. For example in the OBJ viewer example I use drain on the list of triangle meshes. Then you can use the Scene::iter or iter_mut method to iterate through them, since the Scene hangs on to them for you. There's also get_geometry and get_geometry_mut to get a specific geometry by ID. Then if you ever need to move some mesh to another scene when you detach it the Geometry is returned back to you. It kind of works like the scene takes ownership of the geometry while it's attached.

I'm not sure on the others though, the easiest might be to push them into Box's or Arc's so they're on the heap instead of the stack. It's a bit annoying but may work. I would like to have this work as a use case, since I think that's how I'd also likely end up using this library in some future projects. Especially if you want the option to swap out Embree easily for some comparisons. I can also try out pushing these into some Box/Arc wrappers

[Twinklebear]

Seems like the issues are kind of similar to what tomaka was finding with vulkano https://gist.github.com/tomaka/da8c374ce407e27d5dac , his mention about Arc is why I've been trying to avoid introducing them into the main API. That is a pretty old post though, so I wonder if there's been some developments since.

[Twinklebear]

It looks like there's now a Pin type in the Rust standard lib for making these kinds of self-referential structs: https://doc.rust-lang.org/std/pin/index.html , I wonder if this will work here? Then just your EmbreeAcceleration would need to be put behind a Box or Arc (since moving it would invalidate the internal refs) but it should be possible to create one of these objects, but that's probably ok compared to having each member in a Box or Arc.

[beltegeuse]

Hi Will, Thanks for all these answers! I am still not sure how to use Pin but I will figure out. I will close the issue for now and re-open if necessary. Thanks for your hard work on embree-rs :)

[Twinklebear]

Let me know how it goes with Pin, I'm curious how it works as well since I've run into similar issues in Rust. It seems like Pin is designed to solve this, but I also just learned of it in searching around for this issue, so am also not sure how to use it.

[kubo-von]

Hey @Twinklebear and @beltegeuse , I've ran into the same issue while trying to multithread rendering using embree-rs. I was wondering if any of you find some solution in the end ? I've tried putting the scene into pinned boxes and arcs but it does not seem to change anything. Cheers Jakub

[beltegeuse]

Hi @kubo-von,

Do you want put all the different embree_rs objects inside a same struct like:

pub struct EmbreeAcceleration<'embree> {
    pub device: &'embree embree_rs::Device,
    pub scene: embree_rs::CommittedScene<'embree>,
}

Because, at my best knowledge, this is not possible with Rust yet. Indeed, you cannot self-referential lifetime inside the same struct. For more information, you can look at this post: https://users.rust-lang.org/t/how-do-i-create-self-referential-structures-with-pin/24982/11

The only plausible workaround I see is to have an Arc pointer inside the CommitedScene object so that you can get rid of lifetime dependency. I think this is an acceptable solution as you only need to access the device a few times. But as @Twinklebear said before, introducing Arc inside the library might be not ideal.

[kubo-von]

Hi @beltegeuse, I'd like to do that eventually, to make my code more clean and modular. But right now I'm just trying to multithread my rendering process, so I need all the threads to access the same commited embree scene. What I've done is:

• put the commited embree scene in new Arc
• clone the arc, start a new rendering thread using thread::spawn(move ||{....}) which moves the clonned arc into the closure.

But that does not work, because of the life time of embree device which is referenced inside the commited scene, I assume this is the same issue that prevents it to be in the same struct. So I was just wondering if you've managed to solve your life-time issue in the end or if you've ever sucessfully multithreaded the rendering process with embree in rust ?

By having Arc inside the library, you mean forking embree-rs and modifying in a way that there's an Arc instead of reference ?

Thanks! (I'm new to Rust and this level of programming in general so sorry if my questions are a bit dull)

[beltegeuse]

Hi @kubo-von,

You only need to keep borrowing when using a committed scene (in the usual scenario). Indeed, usually, you do not need to update the scene information when achieving rendering, so having read-only access to the committed scene is not an issue.

I have attached a modified version of "obj_viewer" that you can found inside the examples directory. Here I use rayon to achieve parallelization over image rows. You should decompress the archive inside examples directory if you want to built it.

obj_viewer_parallel.zip

[Twinklebear]

@beltegeuse the parallel OBJ viewer with rayon is really cool! Would you mind opening a PR to include it as an example in this repo?

[beltegeuse]

@Twinklebear Sure I can do that. Or maybe an AO integrator? (as primary ray intersection is usually quite fast with Embree). What do you think?

[Twinklebear]

Yeah @beltegeuse an AO integrator would be a great example to include!

[kubo-von]

Thanks a lot for the example @beltegeuse ! That'll be a huge help.

[kubo-von]

So the rayon worked great, but I wanted more control over the number of threads using Threadpool and I've run into the lifetime issue again, with embree device and embree scene not living long enough. It the end the Leak Crate solved it for me. By putting the embree device and the embree scene (before commiting) into Boxes and calling .leak() on them the lifetime issue was solved. Not sure how safe/smart solution it is but might help someone.

[beltegeuse]

Hi @kubo-von,

Just FYI, you can also control the number of threads used by rayon via rayon::ThreadPoolBuilder. Still, I agree that rayon might be too high level on what you are trying to do.

Concerning the approach you took (via leaking), it is fine, but you need to be aware that you create a memory leak (which will be fine in our case, I believe).

PS: I believe that the leak create is not necessary as Box::leak is inside the standard library (https://doc.rust-lang.org/std/boxed/struct.Box.html#method.leak)

[Twinklebear]

Re-opening this issue since I'd like to make some other updates and breaking changes in the API (Embree 3.7 added support for putting a Geometry in multiple scenes which fixes some complaints and issues I had).

I was thinking about this a bit, and was wondering your thoughts about the possible change that I think would fix the life time issue:

• Instead of Device::new returning a Device it returns an Arc<Device> so now everything can keep the device alive as needed instead of needing to track a lifetime. I think this should fix the main issue with the device lifetime kind of polluting everywhere.

• I'd change the Scene vs CommittedScene so that committing the scene would consume it, essentially moving it into a CommittedScene, so the API would be like:

let scene = embree::Scene::new(...);
// set up the scene
let committed_scene = embree::scene::commit(scene);
// trace some rays
committed_scene.intersect(...);

// Then if we want to modify it again we "unfreeze"/"uncommit"
let scene = embree::scene::unfreeze(committed_scene);
// Update the geometry

Or is this just kind of overthinking things and it'd be best to just have commit operate on the scene and not return some new structure? I had it this way to kind of add some safety via the type system so you couldn't call intersect on a scene that hadn't been committed, but Geometry actually also have a commit method that just operates on them: https://github.com/Twinklebear/embree-rs/blob/master/src/geometry.rs#L39 so it's kind of inconsistent with itself.

[Twinklebear]

It actually looks like going w/ Arc's throughout will be the easiest for tracking lifetimes and ensuring things aren't released while they're still being used. I've started on this: https://github.com/Twinklebear/embree-rs/commit/b849ac6fa21db2a211944aa09773e35878364857 as part of the example update

[beltegeuse]

Hi Will, I agree with you. It will be simpler to use Arc to keep track of lifetimes easy. I wonder if these changes will affect (or not) how to use rayon and this crate.

I like the unfreezing option that you are proposing. It is important for correctness to have two different structs (Scene vs. CommitedScene). Indeed, this reduces the error when using this crate (i.e., forgetting to commit a scene, trying to modify a committed scene, ...).

Thanks for your work, Best!

[matthiascy]

It actually looks like going w/ Arc's throughout will be the easiest for tracking lifetimes and ensuring things aren't released while they're still being used. I've started on this: b849ac6 as part of the example update

I think we don't really need Arc, RTCDevice is already reference counted by Embree C side, we only need to wrap the device with rtcRetainDevice, rtcDeleteDevice. Like, impl. Clone for Device and invoke rtcRetainDevice right before copy RTCDevice as it is copyable. Thus when ever we need the device, just call .clone() to increase the reference count.

pub struct Device {
    pub(crate) handle: RTCDevice,
}
impl Clone for Device {
    fn clone(&self) -> Self {
        unsafe { rtcRetainDevice(self.handle) }
        Self {
            handle: self.handle,
        }
    }
}

The same applies to RTCBuffer, RTCScene, RTCGeometry and RTCBVH.

[Twinklebear]

I see, I was wondering at the time if/how we could make use of the internal reference counting that Embree does but was maybe struggling with some lifetime issues or something. I don't quite remember. Maybe I was still trying to pass things by ref instead of cloning, and so was still hitting the same self-referencing issue there.

Using the built in reference count w/ this Clone impl would be the best, because adding an Arc on top just adds another layer of indirection and reference counting on top of what Embree's doing internally.

So then for the Drop impl we'd just have an rtcRelease call?

One bit that might be a bit tricky is that by cloning the scene you could have multiple threads operate on the same scene and cause a race condition? Since the compiler sees them as having their own copies of the object? But maybe that's not something we need to enforce, I may have been trying to push too much safety through the API bindings here and that makes it tougher to implement a simpler API that more folks might find useful

[matthiascy]

Yes, for Drop impl, rtcRelease would be enough. Under the hood, rtcRetain increases the ref. count, rtcRelease decreases it. We are free to clone and drop, Embree will do the rest. Basically, we are immitating Rc, and in most use cases, we use only the reference like &Device which is equivalent to &Rc<Device>, then we clone in places where we need to keep a 'copy' (probably behind the API).

According to official responses concerning the thread safety https://github.com/embree/embree/issues/207#issuecomment-418291686 and https://community.intel.com/t5/Intel-Embree-Ray-Tracing-Kernels/rtcNewScene-rtcNewGeometry-thread-safety/m-p/1164216#M795, only Device is thread safe, thus cloning other objects other than Device should be avoided in multi-threading situations.

[AndreiBarsan]

Joining this thread as I have a very relevant question. Thank you for the awesome crate @Twinklebear!!

I have hit the exact same problem as the original issue author: I am struggling to integrate embree-rs into a nontrivial application due to lifetime issues. (I apologize in advance but I am not too experienced with Rust.)

My goal is to abstract away all Embree details behind a trait or something, but this would mean storing the Device and Scenes together at some point, which seems impossible in the current setup (?).

I tried to follow the Arc usage in this branch but I could not get it to work. If we do go the Arc route, wouldn't that make mutating anything a headache? Namely if I create a scene, new would return Arc<Scene> - but I can't mutate this as it's behind an Arc, so how do I actually attach geometries to the scene's geometry HashMap, update their poses, etc.?

[matthiascy]

Hi, @AndreiBarsan,

This issue won't exist on this branch (origin of PR #22). There are still some examples and functions left to implement. Once it's done, there will be a complete version of embree3.