Problem with mutable textures (mutability generally)


#1

I’ve got a project with a simple struct like this:

pub struct RenderState<'a> {
    pub blendMode: BlendMode,
    pub transform: Matrix4<f32>,
    pub texture: &'a Texture<'a>,
    pub shader: &'a Shader,
    pub viewport: Rectangle,
}

pub struct Texture<'a> {
    pub texture: sdl2::render::Texture<'a>,
}

My code at some point is making use of gl_unbind_texture for the SDL2 texture and that function is actually mutating the SDL2 texture:

    pub unsafe fn gl_unbind_texture(&mut self) {
        if ll::SDL_GL_UnbindTexture(self.raw) != 0 {
            panic!("OpenGL texture unbinding not supported");
        }
    }

The compiler is complaining:

  --> src/graphicsdevice.rs:99:13
   |
99 |             state.texture.texture.gl_unbind_texture();
   |             ^^^^^^^^^^^^^^^^^^^^^ cannot mutably borrow immutable field
   |
  ::: src/renderstate.rs
   |
15 |     pub texture: &'a Texture<'a>,
   |                  --------------- use `&'a mut Texture<'a>` here to make mutable

And as much as I know that it makes sense, I cannot figure out how to correctly add mutability to my structs in a way that is correct. All of my attempts ended up in failures :frowning:

Would it make any sense to use Rc instead of plain references?

Can anyone help me pointing out what’s the correct path to follow in such cases?
Cheers!


#2

For the time being I managed to solve this with:

pub struct RenderState<'a> {
    pub blendMode: BlendMode,
    pub transform: Matrix4<f32>,
    pub texture: &'a mut Texture<'a>,
    pub shader: &'a Shader,
    pub viewport: Rectangle,
}

impl<'a> RenderState<'a> {
    fn new(texture: &'a mut Texture<'a>, shader: &'a Shader) -> RenderState<'a> {
        RenderState {
            blendMode: BlendAlpha,
            transform: Matrix4::one(),
            texture: texture,
            shader: shader,
            viewport: Rectangle::new(0.0, 0.0, 0, 0),
        }
    }
}

but I’m definitely not sure this is the right approach.


#3

Rust only allows for one borrow if it’s a mutable borrow, which RenderState now owns. So long as the rest of your code either doesn’t require access to the Texture from the owning variable, or has access to RenderState to use its mutable borrow, this will work.

I think std::cell::RefCell is what you want. I haven’t had occasion to use RefCell myself, though when I do see it used, it’s in conjunction with Rc (e.g. Rc<RefCell<Texture>>). I’m not sure you need Rc here.

pub struct Texture<'a> {
    pub texture: ::std::cell::RefCell<sdl2::render::Texture<'a>>,
}

// in graphicsdevice.rs, you ran code to unbind (according to your compiler error), now it looks like this with RefCell.
fn example (state: &RenderState) {
    // state.texture.texture.gl_unbind_texture();
    let mut texture = state.texture.texture.borrow_mut();

    texture.gl_unbind_texture();
    // mutable reference drops here, and you can borrow_mut() somewhere else as needed.
}

Something like this? https://is.gd/nn47y1


#4

Thanks for the reply. That RefCell solution sounds tempting, but I don’t know what kind of problems I’m going to face when using shared resources. It kinds of feel like a hacky workaround to make it easier to write code but defeating the strength of Rust, so I’d rather try to avoid it if at all possible.

I’m going to keep writing some more code until I can actually test this part of the code on the field and I will get back here to report progress and/or design decisions.

Cheers!


#5

The RefCell will work well with single-thread code. If you’re pursuing a multi-threaded design, then anything shared across threads should be encapsulated in an ::std::sync::Arc<::std::sync::Mutex<YourStruct>>.


#7

Let me illustrate with some code where I am at and what’s my problem with mutability at the moment.

I have the following struct:

pub struct SpriteBatcher<'a> {
    initial_batch_size: i32,
    max_batch_size: i32,
    initial_vertex_array_size: i32,
    renderer: &'a Canvas<Window>,
    graphics_device: &'a GraphicsDevice,
    batch_item_list: Vec<SpriteBatchItem<'a>>, /// The list of batch items to process.
    batch_item_count: i32, /// Index pointer to the next available SpriteBatchItem in _batchItemList.
    index: Vec<i32>, /// Vertex index array. The values in this array never change.
    vertex_array: Vec<VertexPositionColorTexture>,
}

For the time being just ignore the contained structs, as that shouldn’t really be the issue.

Then I have the following function in the impl of SpriteBatcher:

    pub fn draw_batch(&'a mut self, sort_mode: SpriteSortMode/*, Effect effect*/, render_state: &'a mut RenderState<'a>) {
        // nothing to do
        if self.batch_item_count == 0 {
            return;
        }

        // sort the batch items
        match sort_mode {
            SpriteSortMode::SpriteSortModeTexture => self.batch_item_list.sort_by(|a, b| a.cmp(b)),
            SpriteSortMode::SpriteSortModeFrontToBack => self.batch_item_list.sort_by(|a, b| a.cmp(b)),
            SpriteSortMode::SpriteSortModeBackToFront => self.batch_item_list.sort_by(|a, b| a.cmp(b)),
            _ => self.batch_item_list.sort_by(|a, b| a.cmp(b)),
        }

        // Determine how many iterations through the drawing code we need to make
        let mut batch_index: i32 = 0;
        let mut batch_count: i32 = self.batch_item_count;

        // Iterate through the batches, doing short.MaxValue sets of vertices only.
        while batch_count > 0 {
            // setup the vertexArray array
            let mut startIndex: i32 = 0;
            let mut index: i32 = 0;
            let mut tex: Option<&Texture> = None;

            let mut numBatchesToProcess: i32 = batch_count;
            if numBatchesToProcess > self.max_batch_size {
                numBatchesToProcess = self.max_batch_size;
            }
            
            {
                self.ensure_array_capacity(numBatchesToProcess);
            }

            // Draw the batches
            for i in 0..numBatchesToProcess {
                let mut item = &mut self.batch_item_list[batch_index as usize];
                // if the texture changed, we need to flush and bind the new texture
                let shouldFlush: bool = &**item.texture.as_ref().unwrap() as *const _ != &**tex.as_ref().unwrap() as *const _;
                if shouldFlush {
                    self.flush_vertex_array(startIndex, index /*, effect*/, tex, render_state);

                    tex = self.batch_item_list[batch_index as usize].texture;
                    startIndex = 0;
                    index = 0;
                }

                // store the SpriteBatchItem data in our vertexArray
                index = index + 1;
                self.vertex_array[index as usize] = item.vertexTL;
                index = index + 1;
                self.vertex_array[index as usize] = item.vertexTR;
                index = index + 1;
                self.vertex_array[index as usize] = item.vertexBL;
                index = index + 1;
                self.vertex_array[index as usize] = item.vertexTR;
                index = index + 1;
                self.vertex_array[index as usize] = item.vertexBR;
                index = index + 1;
                self.vertex_array[index as usize] = item.vertexBL;

                // Release the texture.
                item.set_texture(None);
            }
            // flush the remaining vertexArray data
            self.flush_vertex_array(startIndex, index /*, effect*/, tex, render_state);
            // Update our batch count to continue the process of culling down
            // large batches
            batch_count -= numBatchesToProcess;
        }
        // return items to the pool.
        self.batch_item_count = 0;
    }

And the signature of flush_vertex_array is the following:

pub fn flush_vertex_array(&self, start: i32, end: i32 /*, Effect effect*/, texture: Option<&'a Texture<'a>>, render_state: &'a mut RenderState<'a>)

As you can see, with let mut item = &mut self.batch_item_list[batch_index as usize]; I’m borrowing one of the element of the vector as mutable.
Next I’m calling self.flush_vertex_array(startIndex, index /*, effect*/, tex, render_state); where self is passed as immutable so that an immutable borrow occurs, but since I mutably borrowed a field of self, this leads to an error.

I would like to find a correct way to write this kind of function in Rust. Any help is really appreciated as I’m banging my head against the borrow checker at the moment.


#8

I think that the item issue can be fixed just by rewriting that function referencing the content of the Vec and assigning to item only afterwards, just like this:

    pub fn draw_batch(&'a mut self, sort_mode: SpriteSortMode/*, Effect effect*/, render_state: &'a mut RenderState<'a>) {
        // nothing to do
        if self.batch_item_count == 0 {
            return;
        }

        // sort the batch items
        match sort_mode {
            SpriteSortMode::SpriteSortModeTexture => self.batch_item_list.sort_by(|a, b| a.cmp(b)),
            SpriteSortMode::SpriteSortModeFrontToBack => self.batch_item_list.sort_by(|a, b| a.cmp(b)),
            SpriteSortMode::SpriteSortModeBackToFront => self.batch_item_list.sort_by(|a, b| a.cmp(b)),
            _ => self.batch_item_list.sort_by(|a, b| a.cmp(b)),
        }

        // Determine how many iterations through the drawing code we need to make
        let mut batch_index: i32 = 0;
        let mut batch_count: i32 = self.batch_item_count;

        // Iterate through the batches, doing short.MaxValue sets of vertices only.
        while batch_count > 0 {
            // setup the vertexArray array
            let mut startIndex: i32 = 0;
            let mut index: i32 = 0;
            let mut tex: Option<&Texture> = None;

            let mut numBatchesToProcess: i32 = batch_count;
            if numBatchesToProcess > self.max_batch_size {
                numBatchesToProcess = self.max_batch_size;
            }
            
            {
                self.ensure_array_capacity(numBatchesToProcess);
            }

            // Draw the batches
            for i in 0..numBatchesToProcess {
                // if the texture changed, we need to flush and bind the new texture
                let shouldFlush: bool = &**self.batch_item_list[batch_index as usize].texture.as_ref().unwrap() as *const _ != &**tex.as_ref().unwrap() as *const _;
                if shouldFlush {
                    self.flush_vertex_array(startIndex, index /*, effect*/, tex, render_state);

                    tex = self.batch_item_list[batch_index as usize].texture;
                    startIndex = 0;
                    index = 0;
                }

                let mut item = &mut self.batch_item_list[batch_index as usize];
                // store the SpriteBatchItem data in our vertexArray
                index = index + 1;
                self.vertex_array[index as usize] = item.vertexTL;
                index = index + 1;
                self.vertex_array[index as usize] = item.vertexTR;
                index = index + 1;
                self.vertex_array[index as usize] = item.vertexBL;
                index = index + 1;
                self.vertex_array[index as usize] = item.vertexTR;
                index = index + 1;
                self.vertex_array[index as usize] = item.vertexBR;
                index = index + 1;
                self.vertex_array[index as usize] = item.vertexBL;

                // Release the texture.
                item.set_texture(None);
            }
            // flush the remaining vertexArray data
            self.flush_vertex_array(startIndex, index /*, effect*/, tex, render_state);
            // Update our batch count to continue the process of culling down
            // large batches
            batch_count -= numBatchesToProcess;
        }
        // return items to the pool.
        self.batch_item_count = 0;
    }

Then I face yet another problem. The function takes &'a mut RenderState<'a> as parameter and then passes it twice to the self.flush_vertex_array(startIndex, index /*, effect*/, tex, render_state); call.
So basically it’s borrowing once to self.flush_vertex_array and then I’m trying to do that again a second time but since it’s already been borrowed it can’t be borrowed again.

I have no idea what the correct way of calling that same function twice should be. Thanks in advance for all the help!


#9

Is there a reason you’re using an explicit lifetime on the self and render_state arguments? That 'a lifetime is longer than you need here, it would seem (although hard to tell without seeing the flush function).

What compilation error are you getting?


#10

The flush function is the following:

    pub fn flush_vertex_array(&mut self, start: i32, end: i32 /*, Effect effect*/, texture: Option<&'a Texture<'a>>, render_state: &'a mut RenderState<'a>) {
        if start == end {
            return;
        }

        let vertexCount: i32 = end - start;
        render_state.set_texture(texture);
        
        self.graphics_device.draw(&self.vertex_array, vertexCount, render_state);
    }

And the errors are the following:

error[E0499]: cannot borrow `*render_state` as mutable more than once at a time
   --> src/spritebatcher.rs:134:82
    |
134 |                     self.flush_vertex_array(startIndex, index /*, effect*/, tex, render_state);
    |                                                                                  ^^^^^^^^^^^^
    |                                                                                  |
    |                                                                                  second mutable borrow occurs here
    |                                                                                  first mutable borrow occurs here
...
167 |     }
    |     - first borrow ends here

error[E0499]: cannot borrow `*render_state` as mutable more than once at a time
   --> src/spritebatcher.rs:160:74
    |
134 |                     self.flush_vertex_array(startIndex, index /*, effect*/, tex, render_state);
    |                                                                                  ------------ first mutable borrow occurs here
...
160 |             self.flush_vertex_array(startIndex, index /*, effect*/, tex, render_state);
    |                                                                          ^^^^^^^^^^^^ second mutable borrow occurs here
...
167 |     }
    |     - first borrow ends here

error: aborting due to 2 previous errors

I’m using an explicit lifetime because I need to set it for the let mut tex: Option<&Texture> = None;


#11

The source for the project I’m messing with is available here: https://github.com/tanis2000/minigame-rust

If you run a cargo build you will see the errors I’m facing at the moment.


#12

But you’re specifically using the 'a lifetime, which is the entire lifetime of the SpriteBatcher value - that’s going to create long-lasting borrows.

Have you tried specifying fewer lifetimes? It seems like flush_vertex_array will need to specify the same lifetime for the texture and render_state, but I’m not sure you need it to be 'a.

I’m on my phone so might be missing something. But, I’d start with not specifying any explicit lifetimes in the function signatures, and then see what the compiler complains about (conflicting requirements, something doesn’t live long enough, etc). Then add lifetimes/bounds based on that.


#13

I think the sdl2 crate requires a lifetime parameter from OP example sdl2::render::Texture<'a>[1].


#14

That’s fine and makes sense, but I’m not sure it needs &'a Texture<'a> rather than, say, &'b Texture<'a> (where 'b is either an elided compiler lifetime or user declared one). Similarly for &'a mut RenderState<'a>


#15

The problem is like an expanding oil stain. The rust-sdl2 crate exposes Texture<'r>. In my own Texture struct I have an sdl2 Texture field and the compiler complains that I need to specify a lifetime for the sdl2 Texture. So I end up with

pub struct Texture<'a> {
    pub texture: RefCell<SdlTexture<'a>>,
}

Next my RenderState struct has a Texture field. If I do not add the lifetime specifier the compiler complains that it’s missing and so on with every struct that contains a RenderState field. Lifetime is like an infection. Once you add it to one struct you’ve got to carry it around to every other struct containing one where it’s been defined. I do not know if I should set two different lifetimes. I don’t even understand why I should manually set a lifetime for the sdl2 Texture as it should be managed by its TextureContext when it’s being created, thus the owner and keeper of the Texture is that TextureContext and I don’t get why a reference to a Texture should have a lifetime specifier.


#16

Sorry, maybe I wasn’t clear. I’m not questioning the generic lifetime parameters of the sdl and your own types. Instead, I’m questioning the use of 'a lifetime parameter (from SpriteBatcher<'a>) in the function signatures. For instance, why is pub fn draw_batch(&'a mut self, sort_mode: SpriteSortMode/*, Effect effect*/, render_state: &'a mut RenderState<'a>) not pub fn draw_batch<'b>(&mut self, sort_mode: SpriteSortMode/*, Effect effect*/, render_state: &mut RenderState<'b>)
Note that I’ve not vetted the 2nd signature above, I’m only using it for illustrating my point (or question, perhaps).


#17

Ah, good point.

I think I got it!

Regarding this point in @tanis’s repo

In src/spritebatcher.rs, the draw_batch function mutably borrows a RenderState and that borrow must live for 'a time…

pub fn draw_batch(&'a mut self, sort_mode: SpriteSortMode/*, Effect effect*/, render_state: &'a mut RenderState<'a>) {
        // ...
        // Iterate through the batches, doing short.MaxValue sets of vertices only.
        while batch_count > 0 {
            // ...
            // Draw the batches
            for i in 0..numBatchesToProcess {
                // ...
                if shouldFlush {
                    self.flush_vertex_array(startIndex, index /*, effect*/, tex, render_state);
                    // ...
                }
                // ...
            }
            // flush the remaining vertexArray data
            self.flush_vertex_array(startIndex, index /*, effect*/, tex, render_state);
            // ...
        }
        // ...
    }

…and when flush_vertex_array is called, it’s given a mutable borrow that must also live for 'a time, which when the loop rolls back around, the next call finds the first call’s borrow is blocking it…yes?

    pub fn flush_vertex_array(&mut self, start: i32, end: i32 /*, Effect effect*/, texture: Option<&'a Texture<'a>>, render_state: &'a mut RenderState<'a>) {
        // ...
        render_state.set_texture(texture);
        self.graphics_device.draw(&self.vertex_array, vertexCount, render_state);
    }

So changing the signature of flush_vertex_arrays as so…

diff --git a/src/spritebatcher.rs b/src/spritebatcher.rs
index 5de053a..2bb602e 100644
--- a/src/spritebatcher.rs
+++ b/src/spritebatcher.rs
@@ -166,7 +166,7 @@ impl<'a> SpriteBatcher<'a> {
         self.batch_item_count = 0;
     }
 
-    pub fn flush_vertex_array(&mut self, start: i32, end: i32 /*, Effect effect*/, texture: Option<&'a Texture<'a>>, render_state: &'a mut RenderState<'a>) {
+    pub fn flush_vertex_array(&mut self, start: i32, end: i32 /*, Effect effect*/, texture: Option<&'a Texture<'a>>, render_state: &mut RenderState<'a>) {
         if start == end {
             return;
         }

…appears to solve the multiple mutable borrow error.

I find it odd that the mutable borrow in draw_batch doesn’t travel into flush_vertex_array and return back to draw_batch to be reused. Instead, it seems like a new “mutable borrow item” is created that lives longer than the function call to flush_vertex_array, whereas my change forces the new “mutable borrow item” to drop when the function returns.

I don’t fully understand it, someone else will have to confirm.

Update: Oh! and the signature for draw_batch can change similarly to limit the lifetime of those borrowed parameters (not the arguments). Those required &'a borrows because of fetch_vertex_array.


#18

Nice job!

When you pass an already borrowed reference to a function, it performs a re-borrow (moral equivalent of & *ref or &mut *ref). In the case of flush_vertex_array, the re-borrow is for the 'a lifetime; with your change, the re-borrow is for the shortest lifetime the compiler can solve for, which will be just the function call itself. Once the function returns, the short re-borrow is over, and the borrow in draw_batch is “returned”. Whereas if you re-borrow for 'a, that re-borrow does not end, and makes it unavailable in draw_batch.

Reborrowing is touched upon a bit in the nomicon: https://doc.rust-lang.org/nomicon/references.html#liveness


#19

You are right. That solves the error but I don’t understand why it solves it.

I didn’t even know you could write it like this. Isn’t the meaning the same, though?


#20

This makes sense now that I think about it, but it’s damn hard to think in those terms and with lifetimes I find it easy to get lost and forget what’s the lifetime of each borrowed field.


#21

Does my previous post help?[quote=“tanis, post:19, topic:11313”]
I didn’t even know you could write it like this. Isn’t the meaning the same, though?
[/quote]
It’s not. The 'a in your first example is the same lifetime from the SpriteBatcher<'a> definition. That definition basically says that a SpriteBatcher is dependent on some user supplied (but precisely determined by the compiler) lifetime called 'a. Specifically, that lifetime is at least as long as the lifetime of the SpriteBatcher itself. If you then use that lifetime in your function signatures, specifically with mutable references, you’ll extend the borrow to a scope that encompasses the entire lifetime of the SpriteBatcher.

Instead, you should decouple your other lifetimes from 'a because you don’t need the borrows to be that long. If in doubt, it’s better to start with not specifying any explicit lifetimes and let the borrow checker solve for the minimal required lifetime on its own. If it can’t, it’ll give you errors that you can use to refine lifetimes (by specifying them explicitly and/or their relationship/bounds).