Rust's std::sync::Arc vs. C++ std::shared_ptr, lifetimes, etc. questions on some example code

I'm in the middle of converting some existing C++ code to Rust (for educational purpose). The crate I'm trying to build is based on the Physically Based Rendering book (PBRT - http://www.pbrt.org).

Let me start with a link to a part of the current documentation for this crate: pbrt - Rust

While a natural representation would be to have a Triangle shape implementation where each triangle stored the positions of its three vertices, a more memory-efficient representation is to separately store entire triangle meshes with an array of vertex positions where each individual triangle just stores three offsets into this array for its three vertices.

And show some of the relevant C++ code:

// Triangle Declarations
struct TriangleMesh {
    // TriangleMesh Public Methods
    TriangleMesh(const Transform &ObjectToWorld, int nTriangles,
                 const int *vertexIndices, int nVertices, const Point3f *P,
                 const Vector3f *S, const Normal3f *N, const Point2f *uv,
                 const std::shared_ptr<Texture<Float>> &alphaMask,
                 const std::shared_ptr<Texture<Float>> &shadowAlphaMask);

    // TriangleMesh Data
    const int nTriangles, nVertices;
    std::vector<int> vertexIndices;
    std::unique_ptr<Point3f[]> p;
    std::unique_ptr<Normal3f[]> n;
    std::unique_ptr<Vector3f[]> s;
    std::unique_ptr<Point2f[]> uv;
    std::shared_ptr<Texture<Float>> alphaMask, shadowAlphaMask;
};

class Triangle : public Shape {
  public:
    // Triangle Public Methods
    Triangle(const Transform *ObjectToWorld, const Transform *WorldToObject,
             bool reverseOrientation, const std::shared_ptr<TriangleMesh> &mesh,
             int triNumber)
        : Shape(ObjectToWorld, WorldToObject, reverseOrientation), mesh(mesh) {
        v = &mesh->vertexIndices[3 * triNumber];
        triMeshBytes += sizeof(*this);
    }
...
  private:
...
    // Triangle Private Data
    std::shared_ptr<TriangleMesh> mesh;
    const int *v;
};

If you want to have a peek at the current Rust code, it is here: GitHub - wahn/rs_pbrt: Rust crate to implement a counterpart to the PBRT book's (3rd edition) C++ code. See also https://www.rs-pbrt.org/about ...

Maybe the example code from pbrt_spheres_differentials_texfilt.rs is most relevant for the questions I have:

...
    let triangle_mesh: Arc<TriangleMesh> = Arc::new(TriangleMesh::new(object_to_world,
                                                                      world_to_object,
                                                                      false,
                                                                      false,
                                                                      n_triangles,
                                                                      vertex_indices,
                                                                      n_vertices,
                                                                      p_ws, // in world space
                                                                      s, // empty
                                                                      n, // empty
                                                                      uv));
    let mut tris: Vec<Triangle> = Vec::new();
    for i in 0..n_triangles {
        tris.push(Triangle::new(object_to_world,
                                world_to_object,
                                false,
                                triangle_mesh.clone(),
                                i));
    }
    println!("tris = {:?}", tris);

If you clone the repository and compile the examples and create ...

cargo test --release

... you can run the resulting example:

./target/release/examples/pbrt_spheres_differentials_texfilt

From the output you will see that I try to create 2 triangles:

n_triangles = 2

With 4 vertices:

n_vertices = 4
p_ws = [Point3 { x: -99.75, y: -1, z: -100 }, ... }]

The TriangleMesh is supposed to hold all the relevant data, and the Triangle to keep a thread-safe reference-counting pointer.

So my question are related to the following two C++ lines:

    std::shared_ptr<TriangleMesh> mesh;
    const int *v;

I decided already about the mesh pointer:

pub struct Triangle {
    mesh: Arc<TriangleMesh>,
}

Question 1: Is this the right decision?

Question 2: What should the v pointer translate to?

I think a slice could be a possible answer. It always would contain three indices into p: Vec<Point3f> from the (shared) TriangleMesh:

tris = [Triangle { mesh: TriangleMesh { n_triangles: 2, vertex_indices: [0, 2, 1, 0, 3, 2], n_vertices: 4, p: [Point3 {...}, ...] ...]

So that print of a vector of two triangles would somehow look like this:

tris = [Triangle { mesh: ..., v: [0, 2, 1] }, Triangle { mesh: ..., v: [0, 3, 2] }]

Can you help me with this? Am I going down the correct route? How would the definition of v look like? I'm assuming I will run into lifetimes syntax problems, but I rather ask now than investing too much time into a direction I should have avoided ...

Thanks. I would appreciate all comments on the current decisions I made. If you want to look at the C++ counter part, here is the C++ code:

I don't know why it doesn't format correctly above:

Rust code: https://github.com/wahn/rs_pbrt

And to repeat the two questions (also not formatted correctly above):

Q: Is this the right decision? (using std::sync::Arc for the mesh pointer)
Q: How would the definition of v look like?

I think that in Rust you can avoid shared_ptr altogether and use a nice Copy handler:

#[derive(Clone, Copy)
struct Triangle<'a> {
   mesh: &'a TriangleMesh,
   id: u32
}

For the v thing, the simplest would be just to store an index instead and recompute it on demand. Not sure why they cache it in pbrt. If this is for performance reasons, you can do something like

#[derive(Clone, Copy)
struct Triangle<'a> {
   mesh: &'a TriangleMesh,
   verts: &'a [u32; 3]
}

and use transmuting to actually construct the Triangle. I think that transmuting a slice into &[u32; 3] should be safe if there's at least three elements in the slice.

Thanks a lot. I tried what you suggested and struggled a bit with the lifetime syntax, but it does compile now:

#[derive(Debug,Copy,Clone)]
pub struct Triangle<'a> {
    mesh: &'a TriangleMesh,
    id: usize,
}

impl<'a> Triangle<'a> {
    pub fn new(object_to_world: Transform,
               world_to_object: Transform,
               reverse_orientation: bool,
               mesh: &'a TriangleMesh,
               tri_number: usize) -> Triangle<'a> {
        Triangle {
            mesh: mesh,
            id: tri_number,
        }
    }
...
}

The output of the example code looks like:

tris = [Triangle { mesh: TriangleMesh {...}, id: 0 }, Triangle { mesh: TriangleMesh {...}, id: 1 }]

The C++ code to access the uv-coordinates (if available) would translate from:

  private:
    void GetUVs(Point2f uv[3]) const {
        if (mesh->uv) {
            uv[0] = mesh->uv[v[0]];
            uv[1] = mesh->uv[v[1]];
            uv[2] = mesh->uv[v[2]];
        } else {
            uv[0] = Point2f(0, 0);
            uv[1] = Point2f(1, 0);
            uv[2] = Point2f(1, 1);
        }
    }

Would translate to Rust (using the triangle ID):

    fn get_uvs(&self) -> Vec<Point2f> {
        let mut uv: Vec<Point2f> = Vec::new();
        if self.mesh.uv.is_empty() {
            uv.push(Point2f { x: 0.0, y: 0.0 });
            uv.push(Point2f { x: 1.0, y: 0.0 });
            uv.push(Point2f { x: 1.0, y: 1.0 });
        } else {
            uv.push(self.mesh.uv[self.mesh.vertex_indices[self.id * 3 + 0]]);
            uv.push(self.mesh.uv[self.mesh.vertex_indices[self.id * 3 + 1]]);
            uv.push(self.mesh.uv[self.mesh.vertex_indices[self.id * 3 + 2]]);
        }
        uv
    }

Thanks again. I guess it will get interesting again once I do multi-threading ...

This should be - > [Point2f; 3] I think. There's a huge performance difference between Vec<X> and [X; n];

Done:

https://github.com/wahn/rs_pbrt/commit/aaa47f109d88a9348beaec9f4e2689f5ead9c90e

Thanks again.

I have another question about lifetimes and the current state of my crate.

Basically this checkin gives me headache:

https://github.com/wahn/rs_pbrt/commit/dd9e385f16c3989f57a24c310a06227681683dec

What I try to do is that I define an interface (in C++ done via inheritance):

https://www.janwalter.org/doc/rust/pbrt/trait.Primitive.html

I figured out how to define a trait (called Primitive) and implement (currently) two functions for two different structs (Sphere and Triangle). Triangle has a lifetime on it because it's sharing a TriangleMesh which is supposed to hold all the relevant data (see above). My example code in question tries to create two triangles (sharing the common triangle mesh) and two spheres, pass those 4 primitives to an acceleration structure BVHAccel which in it's constructor loops through all primitives to call one of the interface functions (in this case world_bound()). It works fine for the 2 spheres (so the checked in code compiles), but as soon as you would uncomment one of this lines:

//primitives.push(Box::new(triangle1));
//primitives.push(Box::new(triangle2));                                                                                                                      

... you would end up with an error message like this:

error: `triangle_mesh` does not live long enough
   --> examples/pbrt_spheres_differentials_texfilt.rs:77:65
    |
77  |         Triangle::new(object_to_world, world_to_object, false, &triangle_mesh, 0);
    |                                                                 ^^^^^^^^^^^^^ does not live long enough
...
253 | }
    | - borrowed value only lives until here
    |
    = note: borrowed value must be valid for the static lifetime...

Can someone help me to modify the existing code to do what I just described? I know it must be related to the usage of Box::new, which seems to be the only stable way to create a Box, but I'm kind of stuck here ... Any suggestions?

I haven't had a chance to look in details at your problem, but I just wanted to mention that I have also been working on a port of pbrt to Rust, which you might find interesting: https://bitbucket.org/abusch/rustracer

I'll definitely be checking out your project, as I'm curious to see how you approached design decisions that I've struggled with!

Looking at my code, it looks like I went with Arc for my Triangle struct. Probably because I couldn't get the lifetimes working otherwise :stuck_out_tongue:

1 Like

Thanks. It looks like you are far ahead. I will have a look at your code and hope I can learn from it.

You probably want to give BVHAccel a generic lifetime parameter and then use it to constrain the lifetime of boxed Primitive trait objects stored in the Vec there. As it stands, you're running into the issue that default lifetime of a trait object is 'static, meaning it must either have no references at all or only 'static references. Your Triangle has a lifetime associated with it, due to holding a mesh ref, and that's why the compiler is complaining that the mesh ref doesn't live long enough once you try to box up the Triangle.

OK. Let's try lifetime first. I tried to follow what @vitalyd suggested and made the following changes to src/lib.rs:

@@ -3694,26 +3694,17 @@ pub enum SplitMethod {
     EqualCounts,
 }

-pub struct BVHAccel {
+#[derive(Clone)]
+pub struct BVHAccel<'a> {
     max_prims_in_node: i32,
     split_method: SplitMethod,
-    primitives: Vec<Box<Primitive>>,
+    primitives: &'a Vec<Box<Primitive>>,
 }

-impl Default for BVHAccel {
-    fn default() -> BVHAccel {
-        BVHAccel {
-            max_prims_in_node: 1,
-            split_method: SplitMethod::SAH,
-            primitives: Vec::new(),
-        }
-    }
-}
-
-impl BVHAccel {
-    pub fn new(p: Vec<Box<Primitive>>,
+impl<'a> BVHAccel<'a> {
+    pub fn new(p: &'a Vec<Box<Primitive>>,
                max_prims_in_node: i32,
-               split_method: SplitMethod) -> BVHAccel { 
+               split_method: SplitMethod) -> BVHAccel<'a> {
         let bvh = BVHAccel {
             max_prims_in_node: std::cmp::min(max_prims_in_node, 255),
             split_method: split_method,

That does compile, but if I want to use it with my example code it fails here:

let accelerator: BVHAccel = BVHAccel::new(&primitives, 4, SplitMethod::SAH);

If I comment out that line the example code compiles as well. Here the diff:

@@ -82,8 +82,8 @@ fn main() {
     println!("uvs[{:?}] = {:?}", 0, uv);
     let uv = triangle2.get_uvs();
     println!("uvs[{:?}] = {:?}", 1, uv);
-    //primitives.push(Box::new(triangle1));
-    //primitives.push(Box::new(triangle2));
+    primitives.push(Box::new(triangle1));
+    primitives.push(Box::new(triangle2));

     // sphere

@@ -249,5 +249,5 @@ fn main() {
                                       pixel_bounds);
     println!("integrator = {:?}", integrator);
     // pbrt::RenderOptions::MakeScene
-    let accelerator: BVHAccel = BVHAccel::new(primitives, 4, SplitMethod::SAH);
+    let accelerator: BVHAccel = BVHAccel::new(&primitives, 4, SplitMethod::SAH);
 }

But as soon as I activate that last line I get an error:

   Compiling pbrt v0.1.0 (file:///home/jan/git/self_hosted/Rust/pbrt)
error: `triangle_mesh` does not live long enough
   --> examples/pbrt_spheres_differentials_texfilt.rs:77:65
    |
77  |         Triangle::new(object_to_world, world_to_object, false, &triangle_mesh, 0);
    |
...
253 | }
    | - borrowed value only lives until here
    |
    = note: borrowed value must be valid for the static lifetime...

What am I doing wrong here? I only found examples with references. What's the lifetime syntax to make this work?

What I meant was primitives: Vec<Box<Primitive + 'a>>, not a reference to the vec.

Same thing, lib.rs compiles:

-pub struct BVHAccel {
+pub struct BVHAccel<'a> {
     max_prims_in_node: i32,
     split_method: SplitMethod,
-    primitives: Vec<Box<Primitive>>,
+    primitives: Vec<Box<Primitive + 'a>>,
 }

-impl Default for BVHAccel {
-    fn default() -> BVHAccel {
-        BVHAccel {
-            max_prims_in_node: 1,
-            split_method: SplitMethod::SAH,
-            primitives: Vec::new(),
-        }
-    }
-}
-
-impl BVHAccel {
+impl<'a> BVHAccel<'a> {
     pub fn new(p: Vec<Box<Primitive>>,
                max_prims_in_node: i32,
-               split_method: SplitMethod) -> BVHAccel {
+               split_method: SplitMethod) -> BVHAccel<'a> {
         let bvh = BVHAccel {
             max_prims_in_node: std::cmp::min(max_prims_in_node, 255),
             split_method: split_method,

But not the example:

@@ -82,8 +82,8 @@ fn main() {
     println!("uvs[{:?}] = {:?}", 0, uv);
     let uv = triangle2.get_uvs();
     println!("uvs[{:?}] = {:?}", 1, uv);
-    //primitives.push(Box::new(triangle1));
-    //primitives.push(Box::new(triangle2));
+    primitives.push(Box::new(triangle1));
+    primitives.push(Box::new(triangle2));

     // sphere

Here the error message:

error: `triangle_mesh` does not live long enough
   --> examples/pbrt_spheres_differentials_texfilt.rs:77:65
    |
77  |         Triangle::new(object_to_world, world_to_object, false, &triangle_mesh, 0);
    |                                                                 ^^^^^^^^^^^^^ does not live long enough
...
253 | }
    | - borrowed value only lives until here
    |
    = note: borrowed value must be valid for the static lifetime...

Make that p: Vec<Box<Primitive + 'a>> :slight_smile: Basically, you need to tell Rust that your trait objects, if they contain any references, will not have any references that live less than 'a, which is also the lifetime of the BVHAccel value that owns the vec holding them. If they did live for a shorter lifetime than 'a, your BVHAccel would potentially be able to access invalid/freed memory.

Thank you so much, @vitalyd ... it works :joy:

1 Like

No problem. I also realized I misspoke in my previous post about who outlives what - I corrected that, and also added another clarifying (I hope) sentence.

1 Like

@wahn one more performance tip! Box<Primitive> does not feel right. Each Box is a separate allocation, and allocations are generally slow.

I think what you really want is Vec<&'scene Primitive>, and you general structure may look like this:

/// A **data** describing the scene. This object does not have lifetimes
/// and is immutable once constructed
struct SceneDescription {
   meshes: Vec<TriangleMesh>,
   spheres: Vec<Sphere>,
   ...
}

/// This is a processed form of `SceneDescription`,
/// which builds indexes, sorts primitives, etc, but
/// does not hold any data itself and references the `SceneDescription`
struct Scene<'scene> {
    primitives: Vec<&'scene Primitive>
}

impl<'s> Scene<'s> {
    fn new(scene: &'s SceneDescription) -> Scene<'s> { ... }
}

fn main() {
    let scene_description: SceneDescription = load_from_disk();
    let scene = Scene::new(&scene_description);
    scene.render(); 
}
1 Like

I have to admit I've been more interested in seeing pretty pictures appear on my screen than fighting with lifetime errors, so I started using Arcs everywhere, but I'm planning to have a look at performance at some point so this is all good stuff! Thanks @matklad and @vitalyd for the tips!

@matklad thanks for your suggestion. I think that's pretty much what the original C++ code does. I started looking into that part of code. I'm avoiding writing a parser for the (similar to Pixar's RIB) scene description, because I don't want to deal with lex and yacc (or flex and bison) yet. But it's clear that there is a parser generator, using an API to store the scene description while loading from disk. Basically a PBRT or RIB file looks like this:

# rendering options
WorldBegin
# scene definition
WorldEnd
# rendering can begin

In C++ the parser will call pbrtWorldEnd() at the very end (when we are basically done with parsing):

// done with parser (for scene description)
void pbrtWorldEnd() {
...
        std::unique_ptr<Integrator> integrator(renderOptions->MakeIntegrator());
        std::unique_ptr<Scene> scene(renderOptions->MakeScene());
...
        if (scene && integrator) integrator->Render(*scene);
...
}
Scene *RenderOptions::MakeScene() {
    std::shared_ptr<Primitive> accelerator =
        MakeAccelerator(AcceleratorName, primitives, AcceleratorParams);
    if (!accelerator) accelerator = std::make_shared<BVHAccel>(primitives);
    Scene *scene = new Scene(accelerator, lights);
...
    return scene;
}

So, I think what you described is: During parsing the data is constructed and collected, after WorldEnd we are ready to render, but we will process the read data by using accelerator structures (as you said, sort primitives, calculate bounding objects, store stuff in accelerators etc.).