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

Texture and texture creator #667

Closed
adfaure opened this issue Jun 2, 2017 · 20 comments
Closed

Texture and texture creator #667

adfaure opened this issue Jun 2, 2017 · 20 comments
Labels
Milestone

Comments

@adfaure
Copy link

adfaure commented Jun 2, 2017

Hello, First of all I am sorry for the newbee question.

I have some trouble to understand how I can manage the Texture and the Texture creator.

I am stuck with the lifetime of the texture. So for the moment I do something like the code bellow to get a texture, but I have to do it at each iteration of my game loop. I have the feeling that is not the way it have to be used.

/// Example of how I create a texture
let map_loader = sdl2::rwops::RWops::from_file(Path::new("assets/Overworld.png"), "r").unwrap();
let surface: Surface = map_loader.load_png().unwrap();
let texture = self.creator.create_texture_from_surface(&surface).unwrap();

So basically I wanted to have a structure that own some information (such as the texture for example) the GameScene:

pub struct GameScene<'a> {
    creator: sdl2::render::TextureCreator<sdl2::video::WindowContext>,
    /// I would create a texture once and store it.
    texture: sdl2::render::Texture<'a>,
   /** Other objects **/
}

So here it is, the code bellow does not compile because the creator outlives the texture.
I have no clue of how to do such a things, I did some attempts:

struct TextureHolder<'a> {
    creator: &'a sdl2::render::TextureCreator<sdl2::video::WindowContext>,
    texture: sdl2::render::Texture<'a>
}

pub struct GameScene<'a> {
    creator: sdl2::render::TextureCreator<sdl2::video::WindowContext>,
    texture: TextureHolder<'a>
}

The code above would allows me to have a texture and its creator bounded to the same lifetime
But when I am creating a GameScene with the following code

let creator = canvas.texture_creator();
let textureHolder = TextureHolder {
    creator: &creator,
    texture: creator.create_texture_from_surface(&surface).unwrap()
};

GameScene{
    /// I have to find someone to own my creator.
    creator: creator,
    grid: grid,
    map: map,
    /// So this 
    textureHolder : textureHolder 
}

I've got the error :

error: `creator` does not live long enough
   --> src\scene.rs:95:23
    |
95  |             creator: &creator,
    |                       ^^^^^^^ does not live long enough
...
105 |     }
    |     - borrowed value only lives until here
    |
note: borrowed value must be valid for the lifetime 'a as defined on the body at 62:81...

I understand that the texture is bound to the creator, but I have no clue how to achieve this. I hope someone could point me to the right direction.

By the time I'll read some documentations about the lifetime :) .

I'm sorry for the inconvenience,
Thank you very much.

@Cobrand
Copy link
Member

Cobrand commented Jun 2, 2017 via email

@adfaure
Copy link
Author

adfaure commented Jun 2, 2017

Hello,
Thank you very much !
There is no rush :) It this time to sleep for me !

I will go on IRC tomorrow, and I will keep this thread up to date.

@Cobrand
Copy link
Member

Cobrand commented Jun 3, 2017

@adfaure
Copy link
Author

adfaure commented Jun 3, 2017

Hi, so I went on IRC to ask my question.
The conclusion, is that it is impossible to achieve what I want to do; or I use the API the wrong way.

I wonder what is the recommended way to obtain/store a texture before rendering it ? And what is the cost of creating a new texture: with creator.create_texture_from_surface for example.

Thank you.

By the way, very interesting stackoverflow thread, thank you;

@Cobrand
Copy link
Member

Cobrand commented Jun 4, 2017

We have several examples using Texture in the examples folder, for this repo, you might be interested in running cargo run --example resource-manager and cargo run --example game-of-life and examine the source code to see how it is used!

@adfaure
Copy link
Author

adfaure commented Jun 4, 2017

Hi thank you for your answer, but still I am not convinced of the resource manager.
For my case it only add a lot of complexity.

For me the API works well as long as the code stays in one file. But I try to split my code into submodules,
I have huge issues with lifetime.

Thanks for the help, I think I will try a higher level library such as piston. I saw that they also use the this sdl2 binding.

@nrxus
Copy link
Contributor

nrxus commented Jun 5, 2017

I have had similar issues and my recommendation is for your main to handle the lifetimes of the dependencies that are meant to last your whole program. What I mean by this is, create the texture creator (and renderer) in your main and pass a reference of the texture creator(do not move the creator, only a reference is needed!) onto your game struct. This way we guarantee that the texture creator outlives any texture that is created within your game (your game goes out of scope before the texture creator) which solves all the lifetime issues you are having. I hope this helps. I do this for both texture creator the TtfContext as they both need to outlast textures or fonts respectively. I hope this helps (:

@tanis2000
Copy link
Contributor

I wonder if it would be possible to remove the need to have an explicit lifetime on the Texture struct, though. That would make the whole lifetime management a lot easier.

@nrxus
Copy link
Contributor

nrxus commented Jun 29, 2017

I would love to find a solution that simplifies the lifetime management of textures. I am unsure how it coule be done given the constraints of sdl2 while keeping the code safe. Safety for me means
no undefined behaviors that could lead to a segfault.

@Cobrand
Copy link
Member

Cobrand commented Jun 29, 2017

The other immediate solution would be to wrap a reference to Renderer/ TextureCreator on every texture, and destroy the Renderer only when all the textures are dropped as well, but the overhead would be non-negligible, plus you may not want to allocate on the heap unnecessarily . Since Window cannot be destroyed before renderer as well, that would mean that a memory leak would potentially lead to a forever-opened window as well, and I'm not sure that's what we want...

I feel like the current way it is implemented is the "Rust" way of doing it, but if you don't agree with that, the SDL2_sys bindings are open and you can totally create your own wrapper that manages Textures differently!

@tanis2000
Copy link
Contributor

The issue with the explicit lifetime on Texture is that as soon as you have some complex structure that needs to access the Texture, you start propagating explicit lifetimes all over your code.

You can see an example at my repo here: https://github.com/tanis2000/minigame-rust
Just look at https://github.com/tanis2000/minigame-rust/blob/master/src/renderstate.rs or https://github.com/tanis2000/minigame-rust/blob/master/src/spritebatcher.rs

Those structs just need to be able to access the Texture while rendering so that I am carrying around a reference to the Texture of the sprite batch item that I computed. This is still manageable, but then I started adding my own implementation of an entity-component-system on top of that and components that carry around an image, like a sprite, now need to reference their texture as well. So I have to propagate the explicit lifetime over there, too. And the main object that is instantiating the TextureCreator is having issues as I need to keep a reference to TextureCreator somewhere but also pass it over to my texture manager (sort of a resource manager for textures).

I know there must be a solution but things have started getting tricky. It's not really a rust-sdl2 issue per se; it's more like an issue with the need to refactor lifetimes as you build more systems into your own engine. That's why I started wondering if there could be a solution to avoid the explicit lifetime on the Texture itself, but I understand it's probably not possible because Rust is actually forcing you to behave the way you should.

@Cobrand
Copy link
Member

Cobrand commented Jul 5, 2017

What we could do is add a feature that would totally remove the lifetimes by removing the fact that Texture are destroyed on Drop. We could let them live indefinitely or manually (via a unsafe fn destroy(self) or something) and SDL_Renderer's drop actually drop the structure behind Texture. It would be technically safe as long as only one Canvas is used, because Texture cannot be used other than with a Canvas. destroywould be unsafe because it would be possible to destroy a Texture after its Canvas, and that would be undefined behavior; therefore the user must be careful to not destroy Textures after their Canvas.

This alternative is a little bit unsafe and requires more end-user control, but it will get rid of the lifetime and be easier to manage in things such as ECS as well.

I'm going to try something with that in mind soon, and if you don't mind @tanis2000 I'd like you to test that upcoming feature to tell me how it sounds. If that goes well, it's going to be in the v0.31.

@Cobrand Cobrand added this to the 0.31 milestone Jul 5, 2017
@tanis2000
Copy link
Contributor

Sure thing! I'm up to test it out and see how it fits.
So, if I understand it correctly, the Canvas is holding all the Textures. Would it be possible to let the Canvas lend the Texture through Rc.clone() to avoid having to use unsafe code?

@Cobrand
Copy link
Member

Cobrand commented Jul 5, 2017

Unless I misunderstood your question, I'm seeing your proposal as "Texture holding a Rc". Correct me if I'm wrong.

The problem I have with Texture holding an Rc of Canvas is that Canvas cannot be dropped unless all Textures have been dropped as well. Since the Canvas can't be dropped, that means that potentially the Window that is linked to the Canvas can't be dropped. The thing is that Window's drop is literally "stop displaying this Window" from the Operating System perspective, meaning that if you have a memory leak with a Texture, you would have your window left opened until that memory leak goes away... Which is at worst at the program's completion.

The program's completion doesn't sound too bad in a single game, but if your application is made of multiple parts and multiple windows, you don't want to see one of the window still opened with no interaction possible whatsoever (you wouldn't even be able to close it since your event loop for this Window would not be active anymore). I agree that having memory leaks is something programmer-related, but it's trickier to debug than lifetimes.

The tricky part is that memory leaks are 100% safe and can happen a lot of the time, especially when dealing with Rcs

That's why I would prefer something slightly unsafe (but you don't even have to use it, you can let Canvas' Drop drop all the textures without worries) if someone needs fine tuning, than something safe but potentially doing something that the programmer doesn't want at all.

@nrxus
Copy link
Contributor

nrxus commented Jul 5, 2017

I don't know how possible this is, and I wish I had the time to test it out, but in my head I had envisioned the possibility of having the rust version of the sdl2 renderer mimic the c version of sdl2 a little closer, and how that might help. By this I mean, in the c version of sdl2, we have this renderer that owns the textures, and it only is in charge of mananging them. The reason why the textures cannot outlive ther renderer, is because the renderer will destroy them all when the renderer is destroyed. I wonder how possible and beneficial it would be to do such a thing within the rust version.

If we mimic'd this in the rust version, I envision we would have some shared data map that is owned (through some internall unsafe code) by the TextureCreator +Canvas + any Texture created. The TextureCreator is in charge of creating the Texture, adding them to the data map, and returning, not the SDL_Texture, but an identifier to it. The Renderer is in charge of, given a texture identifier, being able to locate it in the map, get the SDL_Texture out, and render it. This texture identifier, will have to be able to delete itself from the data map when it goes out of scope, somehow. When the Canvas goes out of scope, it deletes all of the entries from the data map.

This would solve the lifetime issues because now the TextureCreator doesn't care if the texture identifier outlives the Renderer. If the texture identifier outlives the Renderer, well too bad, no one can render it anyway and the real SDL_Texture was destroyed already. If the Renderer is passed an incorrect identifier, then we return an error, since all of those methods already return Result<> I believe.

I am unsure how doable this is, but I think by "enforcing" the C behavior in the Rust wrapper, we might be able to pull something that feels more idiomatic and ergonomic. Some kinks that would need to be worked out is how would this affect current methods on the Texture (e.g., do the texture query)

I am unsure if this added anything to the discussion since it seems you all already have an idea, I just wanted to share a thought I had but have not had the time to implement yet.

@tanis2000
Copy link
Contributor

@Cobrand actually I mean that the Canvas should own a map of Rc<SDL_Texture> so that it can lend the texture to whatever function is in need of a reference through a clone(). It might make things less "unsafe", but then again I'm no expert with Rc so I might be completely off with this.

While I like @nrxus proposal, it would actually make it a little more difficult the management of textures for those not using the SDL2 rendering stuff. As an example, I am just using SDL2 to manage the window and the textures, while all the rendering is done with my own renderer that is using OpenGL ES 2 with no SDL2 involved at that stage except for retrieving the handle to the actual texture.

On the other hand, just like you envisioned the SDL2 renderer being able to pull out the SDL_Texture and use it, if such a mechanic would be directly available from within the Renderer, that would be fine as well. I'd just skip the rendering part and use the Renderer as a texture storage of sorts.

@Cobrand
Copy link
Member

Cobrand commented Jul 6, 2017

This crate stills aims to have minimum impact on performance while being the closest to the source, and in the source the Renderer doesn't own the Textures, it's merely linked. If the Renderer dies, all of the Textures die automatically, and that's it. A HashMap is still pa performance overhead; having to implement that in the default sdl2 crate means that everyone that don't want to use a HashMap won't be able to do it with sdl2 only, they'd have to use sdl2-sys instead.

Having a HashMap<Id, Texture> would be the role of another crate in this case.

@nrxus
Copy link
Contributor

nrxus commented Jul 6, 2017

It depends how you view "owning". From my perspective, if the Renderer is in charge of creating and destroying the textures, I would view it as owning the Textures.

I agree with the performance overhead, though. Hashmaps would be a hit that we might not want people to incur as the default.

@Cobrand
Copy link
Member

Cobrand commented Sep 5, 2017

Fixed by #683, feel free to re-open if that's not enough.

@Cobrand Cobrand closed this as completed Sep 5, 2017
@tanis2000
Copy link
Contributor

That's super! I'm going to switch my master to use this feature as tests worked fine

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

No branches or pull requests

4 participants