Add new graphics layer #722
Conversation
kernel/multicore_bringup/src/lib.rs
Outdated
| @@ -60,11 +60,11 @@ pub static GRAPHIC_INFO: Mutex<GraphicInfo> = Mutex::new(GraphicInfo::new()); | |||
| #[repr(packed)] | |||
| pub struct GraphicInfo { | |||
| /// The visible width of the screen, in pixels. | |||
| width: u16, | |||
| pub width: u16, | |||
There was a problem hiding this comment.
these are private to avoid accidental modification; there are some accessor functions that you can use instead: https://www.theseus-os.com/Theseus/doc/multicore_bringup/struct.GraphicInfo.html#method.width
applications/porthole/src/lib.rs
Outdated
| let buffer_height = rect.height / CHARACTER_HEIGHT; | ||
| let (x, y) = (rect.x, rect.y); | ||
|
|
||
| let some_slice = slice.as_bytes(); |
There was a problem hiding this comment.
Is there a particular reason why you're converting the string to a byte slice here? I would think it's better to keep it as a string so you can more easily handle wide or multi-byte characters (e.g., emoji, other unicode chars). Reducing it to a byte slice removes important information.
There was a problem hiding this comment.
That's how old api worked to support basic text displaying, supporting multi-byte characters is future work.
applications/porthole/src/lib.rs
Outdated
| buffer: BorrowedSliceMappedPages<u32, Mutable>, | ||
| } | ||
| impl FrameBuffer { | ||
| fn init_front_buffer() -> Result<FrameBuffer, &'static str> { |
There was a problem hiding this comment.
I think it's a good idea to use two separate types for this:
- A type to represent the real physical framebuffer (or framebuffers, in the case of double buffering) that is mapped to actual graphics memory
- A type to represent virtual framebuffers that are just arbitrary chunks of memory
The idea is that, by design, we can prevent accidentally reading from a real framebuffer by not implementing functions that do that, like get_pixel(). Only the virtual framebuffer type would allow reading from it, while the real/physical framebuffer type would only allow writing to it.
applications/porthole/src/lib.rs
Outdated
| for y in rect.start_y()..rect.end_y() { | ||
| for x in rect.start_x()..rect.end_x() { | ||
| if x > 0 && x < self.width as isize && y > 0 && y < self.height as isize { | ||
| self.draw_something(x, y, 0xF123999); |
There was a problem hiding this comment.
one of the main goals of an API for drawing things should be to allow the callers to easily avoid the expensive bounds checks and calculations of pixel offsets on a per-pixel basis. Here, you're forcing that to occur not just once, but twice per pixel, which is a lot of overhead.
Instead of using indexing (which requires bound checking), use iterators.
ouz-a
force-pushed
the
pr-branch
branch
2 times, most recently
from
f1ca63a to
e5b5928
Compare
applications/porthole/src/lib.rs
Outdated
| } | ||
|
|
||
| pub fn display_window_title(window: &mut MutexGuard<Window>, fg_color: Color, bg_color: Color) { | ||
| if let Some(title) = window.title.clone() { |
There was a problem hiding this comment.
this eagerly clones the whole title String every time. Instead move the .clone() operation into the inside of the branch
applications/porthole/src/lib.rs
Outdated
| } | ||
|
|
||
| pub fn print_string( | ||
| window: &mut MutexGuard<Window>, |
There was a problem hiding this comment.
I assume here you mean &mut Window, since there's no real reason to take a mutable reference to a MutexGuard. The only reason you'd pass a guard is if you needed the lock to be released within this function, and if that was the case, you'd need to pass it by ownership
applications/porthole/src/lib.rs
Outdated
| } | ||
|
|
||
| /// FrameBuffer with no actual physical memory mapped, | ||
| /// used for Window and WindowManager's backbuffer. |
There was a problem hiding this comment.
Note that in the future when we support true double buffering using graphics memory, this will not be true (and thus this type shouldn't be used to represent that backbuffer in graphics memory).
applications/porthole/src/lib.rs
Outdated
| let w_it = window.return_framebuffer_iterator(); | ||
| let f = buffer_indexer(&mut self.buffer, self.width, window_screen); | ||
|
|
||
| for (w_color, pixel) in w_it.zip(f) { |
There was a problem hiding this comment.
this is the right idea and, ultimately, the end point for what i was hoping to see once you have obtained an iterator over the window region (source) and an iterator over the framebuffer region (destination).
applications/porthole/src/lib.rs
Outdated
| fn draw_unchecked(&mut self, x: isize, y: isize, col: Color) { | ||
| let x = x; | ||
| let y = y; | ||
| unsafe { |
There was a problem hiding this comment.
definitely no need to use unsafe here or anywhere else in this entire crate.
applications/porthole/src/lib.rs
Outdated
| } | ||
|
|
||
| // We don't want user to draw on top a border | ||
| pub fn return_drawable_area(&self) -> Rect { |
There was a problem hiding this comment.
consider caching the values of rectangular bounding boxes for things like the title rectangle, drawable areas, etc, since they only need to change rarely upon a window resize action.
Then, when the window actually gets resized, you can recalculate them. A great design pattern for this is to use an Option<Rect> with an accessor function like get_or_insert()
applications/porthole/src/lib.rs
Outdated
| }) | ||
| } | ||
|
|
||
| fn copy_windows_into_main_vbuffer(&mut self, window: &mut MutexGuard<Window>) { |
There was a problem hiding this comment.
this should really be a Window method
| fn copy_windows_into_main_vbuffer(&mut self, window: &mut MutexGuard<Window>) { | |
| fn copy_into_vbuffer(&mut self, vbuf: &mut VirtualFrameBuffer){ |
applications/porthole/src/lib.rs
Outdated
| } | ||
| } | ||
|
|
||
| fn draw_unchecked(&mut self, x: isize, y: isize, col: Color) { |
|
Closing in favor of #823 |
Porthole
Porthole, is new graphics layer for the Theseus OS, inspired by the old graphic's stack written from ground up Porthole has several goals:
Design
Detailed design of Porthole will be written here.
Tasks:
This is very early phase code most of the it going change after reviews/discussions