Dropping Instance in the wrong order causes a crash on exit

[Jhynjhiruu]

Hi there,
I've had a strange crash in my project for the past while, in which the program crashes while dropping the Targets for the top screen. I wasn't able to figure out exactly what's going wrong, until I noticed the difference between my program and the example triangle.rs - my code was creating the Instance object after the Targets, and triangle.rs was creating it before. Given what I know about a Target being bound to an Instance, this makes a weird kind of sense - initialising the Target before the Instance means that Rust will drop the Instance first, and the instance calls C3D_Fini when it's dropped, which presumably does something weird. (The crash itself is in the call to free within the C code, so presumably this also manifests using the C toolchain, though I didn't test that.)
I was able to fix the crash pretty simply, by changing the Instance to be initialised first. Is there a better fix? Either unlinking the bound Target on drop somehow, or something fancy with lifetimes to ensure that the Instance must outlive the Target attached to it?
(For a simple repro of the crash, move line 61 of triangle.rs to line 83 (other places may also work), then build and run it on a real 3DS. Citra doesn't care about the crash, since it's on exit, but on a real 3DS, it means exiting to the Homebrew Menu or the HOME Menu isn't possible, which sucks a lot for testing.)

[ian-h-chamberlain]

This is a really good find! I looked at the citro3d code a bit and it looks like C3D_RenderTargetDelete does modify the global render queue, whether the target is linked or not, so I suppose we will need to make sure the Instance lives at least as long as all Targets to avoid double-frees or whatever kind of crash you're seeing.

I can think of 2-3 possible solutions offhand:

• Keep the Instance (or something like it) around with a reference count, like Rc<RenderQueue> or something, and avoid calling C3D_Fini until the last reference is dropped.
• Use something similar to ctru-rs' ServiceReference, which is similar to the previous option, except with a static mutex and error handling etc. This might be good if we want to make Instance thread-safe, but I think is more complicated than necessary if everything stays single-threaded.
• Make Target<'instance> lifetime bounded to the lifetime of the Instance itself. This has the downside that all the APIs we have taking &mut Instance would need to become &Instance. I'm leaning away from this since I prefer to mark things that modify the global state as &mut...

I think I have a proof-of-concept for the first option, so I'll try to put up a PR making that change. In the meantime at least it sounds like you can enforce drop order manually to avoid the crash?

[Jhynjhiruu]

I'm not sure if it's actually explicitly defined as such (it seems like the kind of thing that would be, but I can't find it immediately), but in practice, local variables are dropped in reverse declaration order if they're not otherwise linked. That means you can just declare the instance before any of the targets, and then they'll all be dropped first.
It would be pretty useful if Instance were thread-safe. The reason I'm working on this is that I'm trying to wrangle Bevy into working with my friend, and she's mentioned that we'll probably need Instance to be Send + Sync in order to avoid having to mangle Bevy's internals too badly. And, hopefully, LLVM can optimise it in the single-threaded case.
For now, it's not a hugely high-priority issue, but it would be nice to get it sorted out for the future.

[ian-h-chamberlain]

I'm not sure if it's actually explicitly defined as such (it seems like the kind of thing that would be, but I can't find it immediately), but in practice, local variables are dropped in reverse declaration order if they're not otherwise linked. That means you can just declare the instance before any of the targets, and then they'll all be dropped first.

I think this would be the relevant reference page: https://doc.rust-lang.org/reference/destructors.html#drop-scopes

You can also explicitly drop() the variables to enforce the correct order, I think – but I definitely agree that it would be preferable to just Do The Right Thing automatically instead.

It would be pretty useful if Instance were thread-safe. The reason I'm working on this is that I'm trying to wrangle Bevy into working with my friend, and she's mentioned that we'll probably need Instance to be Send + Sync in order to avoid having to mangle Bevy's internals too badly. And, hopefully, LLVM can optimise it in the single-threaded case. For now, it's not a hugely high-priority issue, but it would be nice to get it sorted out for the future.

Very cool! I had previously started attempting to write some Bevy plugins with this toolchain at https://github.com/ian-h-chamberlain/bevy_3ds/ but I never got around to any graphics stuff because I wanted to flesh out the citro3d-rs API a bit more. I ended up using NonSend<T> for a lot of the ctru structs (like Gfx etc.) and it seemed to work okay, but I haven't really tested it out with a proper game example.

I'm definitely interested in making Bevy a use case for this toolchain, so I'll be curious to hear about your progress integrating citro3d. Do please file issues if you run into them or have feature requests!

[Jhynjhiruu]

I'm not sure if it's actually explicitly defined as such (it seems like the kind of thing that would be, but I can't find it immediately), but in practice, local variables are dropped in reverse declaration order if they're not otherwise linked. That means you can just declare the instance before any of the targets, and then they'll all be dropped first.

I think this would be the relevant reference page: https://doc.rust-lang.org/reference/destructors.html#drop-scopes

You can also explicitly drop() the variables to enforce the correct order, I think – but I definitely agree that it would be preferable to just Do The Right Thing automatically instead.

It would be pretty useful if Instance were thread-safe. The reason I'm working on this is that I'm trying to wrangle Bevy into working with my friend, and she's mentioned that we'll probably need Instance to be Send + Sync in order to avoid having to mangle Bevy's internals too badly. And, hopefully, LLVM can optimise it in the single-threaded case. For now, it's not a hugely high-priority issue, but it would be nice to get it sorted out for the future.

Very cool! I had previously started attempting to write some Bevy plugins with this toolchain at https://github.com/ian-h-chamberlain/bevy_3ds/ but I never got around to any graphics stuff because I wanted to flesh out the citro3d-rs API a bit more. I ended up using NonSend<T> for a lot of the ctru structs (like Gfx etc.) and it seemed to work okay, but I haven't really tested it out with a proper game example.

I'm definitely interested in making Bevy a use case for this toolchain, so I'll be curious to hear about your progress integrating citro3d. Do please file issues if you run into them or have feature requests!

I think that's the right reference page, but I couldn't quite parse out whether it explicitly defines the order for local variables defined in the same scope. It's not hugely important, either way.
Edit: my friend has pointed out the very obvious sentence right at the top of that section that defines it. Bleh.

We've essentially been working on the idea that we need to have graphics working before we handle anything else, so most of our focus has been going towards getting citro3d-rs to talk to Bevy. It's going... well? I think? (It's related to what I was talking about in the ctru-rs issue - figuring out how to get a textured shape rendering was a big chunk of the work, and I think it could work fairly well standalone as an example of how to do that in citro3d-rs.) I'll be sure to keep you updated!
On that note - what license is your work in that repo intended to be under? It'd be very useful to take inspiration from it when the time comes.

[ian-h-chamberlain]

On that note - what license is your work in that repo intended to be under? It'd be very useful to take inspiration from it when the time comes.

Thanks for the reminder — I had originally kept it as a private repo while I was hacking on it and only made it public after it seemed relevant to your efforts here. I've updated the repo to reflect its dual MIT/Apache-2.0 license, which seems like the most obvious choice given the rest of the Bevy ecosystem and this repo.

(It's related to what I was talking about in the ctru-rs issue - figuring out how to get a textured shape rendering was a big chunk of the work, and I think it could work fairly well standalone as an example of how to do that in citro3d-rs.)

Definitely! I had been putting off some of these kinds of "basic" milestones as I tried to cover a lot of the citro3d APIs, but it's high time for some more practical examples and real use cases.

In case you hadn't seen it and find it useful, #20 also includes another use case of rendering an actual mesh, which would probably make a good example too, but I hadn't got around to porting it to safe APIs yet.

[Jhynjhiruu]

In case you hadn't seen it and find it useful, #20 also includes another use case of rendering an actual mesh, which would probably make a good example too, but I hadn't got around to porting it to safe APIs yet.

Yeah, I saw that a little while ago. Unfortunately, the main bit of rendering a model it doesn't touch, textures, is the bit I'm having difficulty with. I've successfully got a shape with a texture, but I find myself hopelessly lost trying to understand how the texture pipeline is supposed to work and I had to go a fair bit against my understanding of how it should work in order to get two faces with two different textures.
Please enjoy this square, which took me about 9 hours to get textured:

PXL_20231223_094143210.mp4

(Sorry for the terrible recording quality. I should have turned down my screen brightness first.)
The problem I'm having is that the texture pipeline seems to persist state across draw calls (Instance::draw_arrays calls, really), even when I think it shouldn't be - first, enabling textures for only the first face and resetting the TexEnv pipeline for the second did nothing, giving me a lovely yellow square (texture coordinates (0.0, 0.0) on Bowser's face) on the back; and second, using two separate textures for the two sides gave me Peach on both sides, for some reason (the Peach texture got used for both sides instead of only the second side, as if the GPU were batching the draws and preserving only the last-written texture). I worked around the issue by merging the textures into one, using it for both draw calls, and adjusting the texture coordinates to match.
I suppose I can technically deal with this by merging all the textures for everything that's supposed to be drawn on one frame into one giant texture and using that for every shape that frame, but it feels like I'm misunderstanding something fundamental about how textures are supposed to work. Either that or it's a bug.
(By the way, having a safe wrapper for draw_elements would be pretty handy in the future for optimising the number of vertices.)

[Jhynjhiruu]

...but it feels like I'm misunderstanding something fundamental about how textures are supposed to work. Either that or it's a bug.

My friend figured it out, and in retrospect it's sort of obvious - textures have to be alive until they're drawn, and if they're freed before the end of the frame, strange things happen. TexEnvs still don't work how I think they do, but at least I can draw more than one texture at a time now.

[ian-h-chamberlain]

I've opened a new issue for the texenv thing, since that feels like something we might be able to encode into a safe Rust API (and we should if possible!), but let's keep this issue for tracking the render target stuff.

For now, I think I'm inclined to try the Rc<RenderQueue> method for fixing this particular crash, and we can try to improve thread-safety of the Instance and stuff later on, if that seems agreeable?

[Jhynjhiruu]

I don't mind either way - whatever is nice to implement works for me.
My friend and I have been experimenting a bit with our own implementations, and we discovered that, relatedly, Programs need to be used essentially exclusively through Pins or something, since C3D_BindProgram stores a pointer to the Program in the C3D context. Our basic implementation makes bind_program take a Pin<Arc<shader::Program>>, taking ownership of the Program and storing it in the Instance. It's a rather irritating API to represent nicely in Rust.

[ian-h-chamberlain]

My friend and I have been experimenting a bit with our own implementations, and we discovered that, relatedly, Programs need to be used essentially exclusively through Pins or something, since C3D_BindProgram stores a pointer to the Program in the C3D context. Our basic implementation makes bind_program take a Pin<Arc<shader::Program>>, taking ownership of the Program and storing it in the Instance. It's a rather irritating API to represent nicely in Rust.

Yeah in hindsight I suppose that makes sense, since obviously the render code needs a reference to the program in order to use it... maybe it would be safer for bind_program to take an owned shader::Program instead or something?

Mind filing a new issue for this? These are the kinds of things that I think will really become the core of this library so I definitely want to spend some time designing it and coming up with an API that is as safe as possible while still being ergonomic.

[Jhynjhiruu]

Done (#37)

[newdev8]

Hello @Jhynjhiruu, could you please share the source code of your textured square? I'm REALLY struggling to draw a simple image to the screen using the GPU.