Added mutability checks for Gfx

[Meziu]

Related to #19. @AzureMarker

The consoleInit process actually depends (and assumes) changes in the gfx environment, mainly related to framebuffers. By using Rust's mutable reference checks, we can prevent the environment from changing while a Console object is alive. This change has however a problem, in that (as it is written right now) the 2 screens (which would normally be worked with separately in this contexts) are actually both stuck together, since the ctru-rs Gfx implementation puts their control under one struct.

I believe we should separate the Screens, thus to not block the Gfx mutability as a whole, but just singular screen controllers. Maybe internal mutability and moving some of the Gfx methods to the singular screens (like set_framebuffer_format) is the way.

[AzureMarker]

This change has however a problem, in that (as it is written right now) the 2 screens (which would normally be worked with separately in this contexts) are actually both stuck together, since the ctru-rs Gfx implementation puts their control under one struct.

I believe we should separate the Screens, thus to not block the Gfx mutability as a whole, but just singular screen controllers. Maybe internal mutability and moving some of the Gfx methods to the singular screens (like set_framebuffer_format) is the way.

I'm not sure I follow this. I think you're pointing out that the Gfx struct (and libctru's gfx API by association) affect both screens? I guess I haven't looked too much at libctru's gfx API to see if you can set settings per-screen (based on the selected screen?), but if so then the second paragraph makes a little more sense.

There also seem to be some functions that can run while the console is around, and they work kind of as expected (ex. wide mode toggle, though the width doesn't get updated in console so it wraps at the old width). Is this something we need to support, or is it not how the libctru API is expected to be used? I guess those might be the methods we add under Screen.

Though also we should make sure we're still supporting non-Screen based graphics work. For example set_framebuffer_format is probably not just for Screens, so we shouldn't hide it behind owning a Screen object.

Edit:

The consoleInit process actually depends (and assumes) changes in the gfx environment

Maybe there's a missing "no" before "changes", and that was tripping me up?

[Meziu]

Ok, I wasn't clear. By my "assumes changes", I mean a Console makes some changes to the gfx environment when it is initialized.

This is mainly about: the screen's framebuffer format (the framebuffers are screen specific, so putting those methods under them isn't an issue), the screen's double buffering mode (it must be off for a Console to work properly), and the wide mode check (as you said, it doesn't work properly because the Console cannot detect a change after init).

These are all screen specific features, that must not change in state while the Console exists. This is why I want to use the mutability checks in the first place. Though, as I stated, right now they are effected globally by a single Gfx instance, and that's why I believe we should separate them.

[AzureMarker]

This is mainly about: the screen's framebuffer format (the framebuffers are screen specific, so putting those methods under them isn't an issue), the screen's double buffering mode (it must be off for a Console to work properly)

These are all screen specific features

I was typing up something here to argue that these are not Screen specific features but Gfx features, when I realized I was confusing Screen and Console types. We only have a Screen enum at the moment, but if we made that into a full struct like Console then having those gfx methods on it would make more sense. My concern was tying these screen APIs behind owning a Console (which in hindsight isn't what you were talking about, sorry).

Maybe we could do something like this: Add top_screen and bottom_screen methods to Gfx which return Screen references (we could have immutable and mutable versions, or use something like Arc to avoid lifetimes). Then prevent the user from creating their own Screen objects. This way we will only ever have two Screen objects (at least per Gfx object, which we should have only one of anyways). I looked at how libctru's API models this, and I see they use an enum for gfxScreen_t just like our current Screen, and that gets passed in as a param to gfx APIs. So we'd just have to make sure that doesn't clash with our API model.

[Meziu]

That was what I thought of too. Giving methods to the Screen enum will be enough I think.

[AzureMarker]

We'd also want to ensure Screen only runs gfx functions after a Gfx is created. So we'd need it to hold a PhantomData<&'gfx Gfx> like Console does. So it probably can't be just an enum anymore.

Never mind. If we only expose it via methods on Gfx we don't need to worry about this. We would still though want to make it opaque so the user can't make their own instances. So we'd need a struct still I think.

[Meziu]

Why do that, can't we just make 2 screen instances as private members of the Gfx struct and pass them as Arcs or something else to the structs (like Console) that need them? I didn't write any code about this idea yet, so I'm sorry if I'm talking without thinking much about the issue.

[AzureMarker]

With that approach, how would a user set a graphics option on a screen, like double buffering?

[AzureMarker]

One idea:

pub struct Gfx {
    pub top_screen: Screen,
    pub bottom_screen: Screen,
}

// Make it impossible for users to create a screen of their own
pub struct Screen(ScreenKind);
pub enum ScreenKind { Top, Bottom }

let mut gfx: Gfx = Gfx::init().unwrap();

// Gets a &mut Screen and calls a method on it.
gfx.top_screen.set_wide_mode(true);

// Initializes consoles using both screens.
let top_console = Console::init(&mut gfx.top_screen);
let bottom_console = Console::init(&mut gfx.bottom_screen);

It might be nicer to use public fields on Gfx for screens because then we can take advantage of partial borrows (aka splitting borrows):
https://doc.rust-lang.org/nomicon/borrow-splitting.html

Edit: I changed Console to take a mutable Screen reference. This would prevent making two consoles for one screen. But we could change that if it's not what we want or doesn't align with libctru.

[Meziu]

I don't even know if you can have two consoles for one screen to be honest.

At this point I just wish their API didn't fall apart for a slightly different order of operations. But ranting is useless, so off we go.

[Meziu]

@AzureMarker I've made these changes. Sadly the borrow splitting idea won't work because taking a mutable reference to a member of a struct is still like taking a mutable reference to the whole struct, and thus calls to Gfx methods won't work because of held references.

This changes aren't clean, nor pretty to be honest. Tell me what you think. We'll still have some stuff to do before pushing.

[AzureMarker]

Here's a few changes to simplify things and fix the wide_screen example (The other examples also need fixing, but they're straightforward):

diff --git a/ctru-rs/examples/gfx_wide_mode.rs b/ctru-rs/examples/gfx_wide_mode.rs
index f4ba365..8510dc5 100644
--- a/ctru-rs/examples/gfx_wide_mode.rs
+++ b/ctru-rs/examples/gfx_wide_mode.rs
@@ -1,7 +1,6 @@
 extern crate ctru;
 
 use ctru::console::Console;
-use ctru::gfx::Screen;
 use ctru::services::hid::KeyPad;
 use ctru::services::{Apt, Hid};
 use ctru::Gfx;
@@ -11,9 +10,9 @@ fn main() {
     let apt = Apt::init().unwrap();
     let hid = Hid::init().unwrap();
     let gfx = Gfx::default();
-    let _console = Console::init(&gfx, Screen::Top);
+    let mut console = Console::init(gfx.top_screen.borrow());
 
-    println!("Press A to enable/disable wide screen mode.");
+    println!("Press A to enable/disable wide screen mode. Extra words here so the text wraps.");
 
     while apt.main_loop() {
         hid.scan_input();
@@ -23,7 +22,17 @@ fn main() {
         }
 
         if hid.keys_down().contains(KeyPad::KEY_A) {
-            gfx.set_wide_mode(!gfx.get_wide_mode());
+            drop(console);
+
+            let mut top_screen = gfx.top_screen.borrow_mut();
+            let wide_mode = top_screen.get_wide_mode().unwrap();
+            top_screen.set_wide_mode(!wide_mode).unwrap();
+            drop(top_screen);
+
+            console = Console::init(gfx.top_screen.borrow());
+            println!(
+                "Press A to enable/disable wide screen mode. Extra words here so the text wraps."
+            );
         }
 
         gfx.flush_buffers();
diff --git a/ctru-rs/src/gfx.rs b/ctru-rs/src/gfx.rs
index 13be457..a18a9ac 100644
--- a/ctru-rs/src/gfx.rs
+++ b/ctru-rs/src/gfx.rs
@@ -1,6 +1,6 @@
 //! LCD screens manipulation helper
 
-use std::cell::{BorrowError, BorrowMutError, Ref, RefCell, RefMut};
+use std::cell::RefCell;
 use std::default::Default;
 use std::ops::Drop;
 
@@ -11,8 +11,10 @@ use crate::services::gspgpu::{self, FramebufferFormat};
 ///
 /// The service exits when this struct is dropped.
 pub struct Gfx {
-    top_screen: RefCell<Screen>,
-    bottom_screen: RefCell<Screen>,
+    pub top_screen: RefCell<Screen>,
+    pub bottom_screen: RefCell<Screen>,
+    // We don't want to allow users to create this struct manually
+    _private: ()
 }
 
 /// Available screens on the 3DS
@@ -51,29 +53,10 @@ impl Gfx {
         Gfx {
             top_screen: RefCell::new(Screen::Top),
             bottom_screen: RefCell::new(Screen::Bottom),
+            _private: ()
         }
     }
 
-    /// Try to get an immutable reference to the Top screen
-    pub fn get_top_screen(&self) -> Result<Ref<'_, Screen>, BorrowError> {
-        self.top_screen.try_borrow()
-    }
-
-    /// Try to get a mutable reference to the Top screen
-    pub fn get_top_screen_mut(&self) -> Result<RefMut<'_, Screen>, BorrowMutError> {
-        self.top_screen.try_borrow_mut()
-    }
-
-    /// Try to get an immutable reference to the Bottom screen
-    pub fn get_bottom_screen(&self) -> Result<Ref<'_, Screen>, BorrowError> {
-        self.bottom_screen.try_borrow()
-    }
-
-    /// Try to get a mutable reference to the Bottom screen
-    pub fn get_bottom_screen_mut(&self) -> Result<RefMut<'_, Screen>, BorrowMutError> {
-        self.bottom_screen.try_borrow_mut()
-    }
-
     /// Flushes the current framebuffers
     pub fn flush_buffers(&self) {
         unsafe { ctru_sys::gfxFlushBuffers() };
@@ -203,6 +186,7 @@ impl Default for Gfx {
         Gfx {
             top_screen: RefCell::new(Screen::Top),
             bottom_screen: RefCell::new(Screen::Bottom),
+            _private: ()
         }
     }
 }

[AzureMarker]

Another idea I had is making separate types for the top and bottom screens. Both could implement one trait for common behavior, but the top screen would have extra methods like set_3d_enabled. This would avoid the current runtime errors and unwrapping that handles if these methods are called on the bottom screen.

[Meziu]

Made the split. It works nicely now, and the structs that want a generic reference to a Screen can use impl or dyn to get it, which makes more sense when writing integrations. This looks good to me, tell me your thoughts too @AzureMarker

[Meziu]

Fixed the problems with the latest commit, including struct access. I had thought RefCell wasn't a safe struct to leave public, but by a close inspection of the docs I noticed there isn't any unwanted behaviour, so now the screen fields are public.

@AzureMarker @ian-h-chamberlain

[AzureMarker]

This should be just be a reference since it doesn't need to be a reference-counted handle:

Suggested change

	pub fn init(screen: Ref<'screen, impl Screen>) -> Self {
	pub fn init(screen: &'screen impl Screen) -> Self {

Also could use dyn here instead of impl, but it doesn't really matter.

[Meziu]

I believe you haven't understood my code, since it's the second time you talked about an Rc implementation. This isn't reference counting, this is internal mutability. Reference counting would bring us to have multiple owners for the Screen, which isn't what we want. This is a RefCell, which is a smart pointer for a mutating object. It's used only for internal mutability inside Gfx, so that we may use the screens/gfx separately.

Also, without using a Ref object, the rules for RefCell's borrowing aren't correctly applied. We may even have to store the reference, now that I think about it.

[AzureMarker]

Sorry, the types are named pretty similar and sometimes I mix them up, especially since Rc<RefCell<_>> is pretty common. Thanks for pointing that out though, I just unconsciously swapped around RefCell and Rc. Also, technically RefCell does actually count references via handles 🙃, just not in the way "reference-counted" implies:
https://doc.rust-lang.org/stable/src/core/cell.rs.html#687

The code as it was when I reviewed it would have functioned the same if you used Ref or &. At that time, the Ref wasn't used, so the only functional thing happening was associating the 'screen lifetime with the returned Console<'screen> instance. Which would work the same with Ref or &.

Now the code has been updated to hold the Ref. But that doesn't really change anything. There's no internal mutability happening here, so removing the RefCell doesn't break anything - Yes, I tested this locally. We should use RefMut instead. That would actually utilize the RefCell by letting you take mutable references to the fields of Gfx (preventing future updates) but still allow &self calls on Gfx.

[Meziu]

Yes, using the lifetime alone led to being able to mutably borrow the Screen even if the Console was using it. NOW it should be all fine. Hope this is the last commit...

@AzureMarker Since the Console holds the Screen anyways, you could also add the set_wide_screen directly to the Console object (and change it to use a RefMut), though that will require testing on which screen it's on (and also some supplementary code to re-init the console in wide-mode with more columns would be cool). That's for another commit though, not this issue

[AzureMarker]

Since the Console holds the Screen anyways, you could also add the set_wide_screen directly to the Console object

The point of passing the screen to the Console via a RefCell type (and it should be RefMut), besides to get the actual gfxScreen_t, is so the screen/gfx can't be modified while the Console exists. If we added this, then you could change the screen width while a console is alive. The console wouldn't get resized (just delegated to the left of the screen), which is probably the least harmful thing that can happen. Of course, maybe this is all well-defined behavior according to libctru and we should allow it anyways...

I did some experiments for if we did decide to do this:

If we did do this, it should be by exposing a method on Console to get an &mut dyn Screen:

pub fn screen(&mut self) -> &mut dyn Screen {
    self.screen.deref_mut()
}

Alternatively, we could include the screen type as a generic on Console and get something pretty cool:

pub struct Console<'screen, S: Screen> {
    context: Box<PrintConsole>,
    screen: RefMut<'screen, S>,
}

impl<'screen, S: Screen> Console<'screen, S> {
    // ...

    pub fn screen(&mut self) -> &mut S {
        self.screen.deref_mut()
    }
}

let mut console = Console::init(gfx.top_screen.borrow_mut());

// If we used the bottom screen this wouldn't compile
console.screen().set_wide_mode(true);

The downside of these two approaches is we can't limit the functions users can call. So if libctru says calling X gfx/screen function is bad if a console is around, but the other functions aren't so bad, then we have limited options. We could instead return an immutable reference to the Screen and make the "bad" functions take a mutable reference. Anyways, like you said, future PR.

[AzureMarker]

NOW it should be all fine. Hope this is the last commit...

Sorry :(, but I think it's really close now, just a few more tweaks, plus making the examples compile.

[Meziu]

console.screen().set_wide_mode(true);

I was thinking at something like a console.set_wide_mode(true). It looks redundant, but it could include setup code to re-init the Console object to have more columns.

[Meziu]

@AzureMarker fixed and added examples. I put hello-world and network-sockets here because they needed changes for the mut-gfx PR anyways, but any changes or refactoring of the code should be done in another issue/PR. For now they just work, including gfx-wide-mode.

[AzureMarker]

console.screen().set_wide_mode(true);

I was thinking at something like a console.set_wide_mode(true). It looks redundant, but it could include setup code to re-init the Console object to have more columns.

Yeah, I haven't looked into this to see if it's possible to re-init the console or manually adjust it, but that should work.

[AzureMarker]

Very close!!! I'm looking forward to this merging. Just a few small nits. The example nits could be done separately like you mentioned. I can open a PR for these to ease the burden on you :).

[Meziu]

Solved all the suggestions