Closed Meziu closed 2 years ago
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
Screen
s, thus to not block theGfx
mutability as a whole, but just singular screen controllers. Maybe internal mutability and moving some of theGfx
methods to the singular screens (likeset_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 thegfx
environment
Maybe there's a missing "no" before "changes", and that was tripping me up?
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.
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.
That was what I thought of too. Giving methods to the Screen enum will be enough I think.
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.
Why do that, can't we just make 2 screen instances as private members of the Gfx
struct and pass them as Arc
s 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.
With that approach, how would a user set a graphics option on a screen, like double buffering?
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.
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.
@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.
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: ()
}
}
}
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.
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
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
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
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.
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.
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.
@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
.
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.
Solved all the suggestions
Related to #19. @AzureMarker
The
consoleInit
process actually depends (and assumes) changes in thegfx
environment, mainly related to framebuffers. By using Rust's mutable reference checks, we can prevent the environment from changing while aConsole
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 thectru-rs
Gfx
implementation puts their control under one struct.I believe we should separate the
Screen
s, thus to not block theGfx
mutability as a whole, but just singular screen controllers. Maybe internal mutability and moving some of theGfx
methods to the singular screens (likeset_framebuffer_format
) is the way.