Rust-SDL2 / rust-sdl2

SDL2 bindings for Rust
MIT License
2.73k stars 468 forks source link

gl context can outlive sdl context #610

Open dagit opened 7 years ago

dagit commented 7 years ago

I just hit a bug in my code where I was unloading textures after sdl (and opengl) had been shutdown already. The obvious fix would be adding lifetimes to the SDL context type and the GL context type with the constraint that the SDL context has to outlive the GL context.

dagit commented 7 years ago

I was able to fix the problem in my code base by using the std::marker::PhantomData on my gl texture id wrapper type. Then in main(), I have a type like this:

  struct Support<'a, 'b: 'a> {
    sdl_context:   &'b sdl2::Sdl,
    event_pump:    &'a mut sdl2::EventPump,
    window:        &'a sdl2::video::Window,
    ...

Basically, all the fields that depend on the SDL context have lifetime 'a and the context itself is given a lifetime of 'b: 'a. I think there is a case to be made for these lifetimes to be part of the types exposed by this library.

Cobrand commented 7 years ago

I don't mind adding a lifetime to GLContext and bumping the version (since this is a breaking change) in the same time, but before rushing this change I'd like to make sure that it is the right thing to do, and that in 100% of the cases you want GLContext to be dropped before SDL. I'm sure there might be hidden cases where a GLContext can work independently without SDL via some dark voodoo magics, and in such a case adding a lifetime would be too restrictive. Other than that, if changing this can allow fellow rust users to avoid the same problem you've had, then sure.

dagit commented 7 years ago

That's a reasonable counter point, more on that in a minute. I suspect it is okay for the gl context to outlive the sdl context, with the caveat that the way the bindings work (and probably sdl itself?) is that the gl context appears to get dropped by the sdl context.

In my case, what I really wanted to express is that the textures ids do not outlive the gl context. I don't think there is any cases where that makes sense. The gl context cleanup will invalidate those textures ids and who knows if that leads to double frees on some gl implementations.

Ignoring the special case of opengl context, it seems like all the sdl subsystems should not be able to outlive sdl itself. Would you agree with that? And then for the special case you mentioned, would it suffice to make it so the opengl context by default cannot outlive the sdl context and then if it turns out to be the wrong assumption we could add an unsafe function that allows the gl context to outlive the sdl context for those users? Basically I'm saying, make the simple/normal case safe while giving power users a way to opt out.

Thanks!

Cobrand commented 7 years ago

That seems like a good solution, the only thing left is implementing it