Add mutable ref to Screen in render::Target

[ian-h-chamberlain]

Closes #3

I think this addresses the issues described there, as the Screen must outlive the render::Target now, and cannot be modified while the Target is alive. I opted to use a plain &mut instead of RefMut as I think it is a little more "standard" and should also continue to work even if we change semantics of Gfx in the future.

CC @Meziu since I guess I can't add you as a reviewer for some reason

[Meziu]

I opted to use a plain &mut instead of RefMut as I think it is a little more "standard" and should also continue to work even if we change semantics of Gfx in the future.

We can’t do this. The reason why we need a RefMut rather than a simple &mut (which is also the reason why we do it this way in ctru::Console) is run-time borrow-checking. Using the .get_mut() call rather than .borrow_mut() is impossible, as it requires the whole Gfx instance to be mutable, since it makes the borrow checks at compile time (you can look at the documentation about it here). We can’t have a mutable Gfx instance at all times since usually both screens would be subject to changes at the same time, causing issues with mutability checks in the struct (double borrows, mostly). This was subject to discussion when we designed the Gfx API.

CC @Meziu since I guess I can't add you as a reviewer for some reason

Yeah, we don’t have administrative roles.

[ian-h-chamberlain]

We can’t do this

Do you mean that we can't change the semantics of Gfx, or that the downstream consumers of Screen can't use &mut? I agree with the former statement, but in this case we are effectively borrowing the screen at runtime (borrow_mut()) and then converting that to a real &mut which can remain for the lifetime of Target. I verified that the following doesn't compile, which I think is our goal here, right?

    let mut render_target = citro3d::render::Target::new(
        width,
        height,
        &mut *top_screen,
        DepthFormat::Depth24Stencil8,
    )
    .expect("failed to create render target");

    top_screen.set_3d_enabled(false);

    let render_target2 = citro3d::render::Target::new(
        width,
        height,
        &mut *top_screen,
        DepthFormat::Depth24Stencil8,
    )
    .expect("failed to create render target");

Also, if you call borrow_mut() twice you just get a panic (as expected), so I don't think there's any danger of somehow double-borrowing the screen, unless I'm just not seeing a case where that's possible?

[Meziu]

The double-borrowing issue isn’t with Screen, it’s with Gfx. Borrowing a struct’s members is equal to borrowing the struct, that’s why RefCel exists at all.

If you really want to know what I mean, try substituting top_screen with bottom_screen in the bottom part (and remove the middle call as it isn’t needed to get my point through).

The issue with compile-time checking is that the whole Gfx struct gets blocked, which means that while having a TopScreen target, no work can be done on the BottomScreen.

[ian-h-chamberlain]

If you really want to know what I mean, try substituting top_screen with bottom_screen in the bottom part (and remove the middle call as it isn’t needed to get my point through).

I've just pushed some changes that I think do exactly what you suggested, and works without any issue on my device. Do you think there's something unsound happening in that program?

I do understand why we need RefCell as members of Gfx, but I still don't think we need the render::Target to hold a RefMut – once you have obtained the RefMut, as far as I understand, it can simply be converted to a &mut and any subsequent borrow() or borrow_mut() will still panic. Am I missing something there?

[Meziu]

If you really want to know what I mean, try substituting top_screen with bottom_screen in the bottom part (and remove the middle call as it isn’t needed to get my point through).

I've just pushed some changes that I think do exactly what you suggested, and works without any issue on my device. Do you think there's something unsound happening in that program?

I do understand why we need RefCell as members of Gfx, but I still don't think we need the render::Target to hold a RefMut – once you have obtained the RefMut, as far as I understand, it can simply be converted to a &mut and any subsequent borrow() or borrow_mut() will still panic. Am I missing something there?

It turns out, I was missing something there -_- . Yes, it is indeed safe to transform the RefMut to an &mut, but this requires the RefMut to be held alive (by something, like your top_screen mutable variable) and for that &mut *top_screen signature to be used. I don't find it very pretty, and I believe a simple borrow_mut would be better, but it doesn't make much of a difference functionally.

@ian-h-chamberlain @AzureMarker We should decide which is better and adapt the APIs accordingly (for the time being, it's just this one and ctru::console).

[ian-h-chamberlain]

for that &mut *top_screen signature to be used. I don't find it very pretty, and I believe a simple borrow_mut would be better, but it doesn't make much of a difference functionally.

Personally, I think an API that takes an &mut is more typical / expected than one that takes a RefMut, but that's just personal preference, I suppose. Maybe @AzureMarker has an opinion to sway us one way or the other.

I realized from this Reddit thread that it should be possible to just use &mut top_screen.borrow_mut(), if the API accepts a concrete type (or we add something like a impl<T> Screen for T where T: Deref<Target = Screen>, which we could do). It's not much better than an explicit borrow_mut().deref_mut() or &mut *borrow_mut(), but it's maybe slightly better?

[AzureMarker]

I'm planning on catching up on this thread soon BTW!
(didn't actually know about this conversation until I was pinged - I'm now watching the repo)

[Meziu]

Though I was thinking, wouldn’t it be better to actually hold the RefMut (rather than a reference to it) since the RefMut has to have a longer lifetime than the reference?

In other words, when we take a screen and pass it to either Console or a citro3d::Target the RefMut has to have a longer lifetime that either of those structs, or at the very least the same. This means that we can’t have those structs hold the reference as much as wanted since we would make sure the RefMut were in scope the whole time (most likely creating problems with multithreaded environments). This way we are basically holding 2 references at once, which doesn’t make much sense.

Still, for the thing about “a more standard API”, you should keep in mind we make the API here, and the user will only ever notice the use of .borrow_mut() rather than &mut. It isn’t a big issue, since users won’t ever be able to “create” Screens themselves.

[AzureMarker]

I realized from this Reddit thread that it should be possible to just use &mut top_screen.borrow_mut()

Ok, my mind is a little blown 🤯:
https://play.rust-lang.org/?version=stable&mode=debug&edition=2015&gist=b2a2d7e8a4e88d90c9190326efd14f3f
Apparently the RefMut lives longer than the statement, and drops at the end of scope?!
That makes the problem of keeping around a RefMut easier, since it's automatic.

In other words, when we take a screen and pass it to either Console or a citro3d::Target the RefMut has to have a longer lifetime that either of those structs, or at the very least the same. This means that we can’t have those structs hold the reference as much as wanted since we would make sure the RefMut were in scope the whole time (most likely creating problems with multithreaded environments). This way we are basically holding 2 references at once, which doesn’t make much sense.

I think the solution from the Reddit thread takes care of holding the RefMut in scope the whole time. But either way, a RefMut is basically a fancy &mut. Both have a lifetime tied to the RefCell.

Another angle that is worth considering is what the user will see when looking at our APIs. They will see that the screens are RefCell<Screen>s, but Target wants a &mut Screen (not the exact types, but it makes the point). The user might be confused how you go from RefCell to &mut. It might be simpler if they instead only need to go from RefCell to RefMut, since there's the RefCell::borrow_mut function which does the conversion in one step.

I agree that using &mut is more standard and maybe simpler, but like @Meziu said, this is mostly a problem for the library internals and we have a lot of control over that (we own the types you can pass, Screen, and how you create them). I could go either way on this issue. I guess if we want to use the same approach for both Console and this, Console benefits more from using RefMut since it's a simple Console::init(gfx.top_screen.borrow_mut()) as opposed to Console::init(&mut gfx.top_screen.borrow_mut()).

Edit: btw, this just made me realize we should probably seal the Screen trait.

[ian-h-chamberlain]

Another angle that is worth considering is what the user will see when looking at our APIs. They will see that the screens are RefCell<Screen>s, but Target wants a &mut Screen (not the exact types, but it makes the point). The user might be confused how you go from RefCell to &mut. It might be simpler if they instead only need to go from RefCell to RefMut, since there's the RefCell::borrow_mut function which does the conversion in one step.

I agree that using &mut is more standard and maybe simpler, but like @Meziu said, this is mostly a problem for the library internals and we have a lot of control over that (we own the types you can pass, Screen, and how you create them). I could go either way on this issue. I guess if we want to use the same approach for both Console and this, Console benefits more from using RefMut since it's a simple Console::init(gfx.top_screen.borrow_mut()) as opposed to Console::init(&mut gfx.top_screen.borrow_mut()).

I thought about this a bit more, and I think we can have an API that works in either case, but I'm not sure the extra complexity is worth it: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=a984f43064bd87b9babbe7d602e936ac

As @AzureMarker pointed out, this is probably somewhat harder to read, if you are a new user wanting to plug and play the Console or citro3d::render::Target types together. That said, if we provide doc examples using borrow_mut(), it might be fairly obvious what the typical way to use these APIs would be.

From all that, I'm inclined to say that maybe just taking a RefMut in both render::Target and Console is the least mental overhead for the end-users, and if we later find use cases for taking plain &mut references we could probably update to an API like the playground above without breaking existing code (and include examples!).

When I get a chance I will push a change to this PR to make it more consistent with the ctru::Console and close out this issue.

[Meziu]

I don’t have much time to pull this and run it locally. @AzureMarker could you give some input on what you think?

Edit: Also, in general, we need a lot of comments. This stuff isn't easy to understand even if the API is nicely designed.

[AzureMarker]

I was able to run it locally, and it's a cool triangle!

[ian-h-chamberlain]

Oops, sorry, I requested review on the wrong PR. I think I'll revisit this one once rust3ds/ctru-rs#76 is settled

[Meziu]

I think I'll revisit this one once rust3ds/ctru-rs#76 is settled

Well, it's time for revisiting! The objective is to handle the Screen inside of citro3d-rs directly, which means that 3d mode and wide mode should also be accessible from within citro3d-rs. We should also update the API to handle modern LINEAR memory and the new Screens. Getting a satisfying citro3d implementation means we can actually get Rust3DS up to standards for homebrew!

[ian-h-chamberlain]

Sorry, I've been a bit inactive lately but I've just pushed some updates based on the feedback! Let me know what you think and hopefully we can get this into place.

[Meziu]

Other than the few things already noted, I find this PR ready to be merged. We can then work on multiple PRs for the different features.

[ian-h-chamberlain]

Guess I'll go ahead and merge this to start working on the next stuf