Add new graphics layer by ouz-a · Pull Request #722 · theseus-os/Theseus

ouz-a · 2022-12-09T12:36:26Z

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:

Make it easy to use.
KISS (E.g; We won't write code that is going increase performance by %2 while making it harder to understand)
Performance is high priority.
It should be modular.

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

kevinaboos

Thanks! I left some comments; the biggest issue is the way that offset calculations and bounds checking happens on a per-pixel basis, sometimes more than once per pixel.

kevinaboos · 2022-12-12T17:49:55Z

 pub struct GraphicInfo {
    /// The visible width of the screen, in pixels.
-    width: u16,
+    pub width: u16,


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

kevinaboos · 2022-12-12T17:53:11Z

+        let buffer_height = rect.height / CHARACTER_HEIGHT;
+        let (x, y) = (rect.x, rect.y);
+
+        let some_slice = slice.as_bytes();


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.

That's how old api worked to support basic text displaying, supporting multi-byte characters is future work.

kevinaboos · 2022-12-12T17:57:00Z

+    buffer: BorrowedSliceMappedPages<u32, Mutable>,
+}
+impl FrameBuffer {
+    fn init_front_buffer() -> Result<FrameBuffer, &'static str> {


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.

kevinaboos · 2022-12-12T17:59:34Z

+        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);


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.

kevinaboos

leaving a partial review to share notes

kevinaboos · 2022-12-29T01:55:54Z

+}
+
+pub fn display_window_title(window: &mut MutexGuard<Window>, fg_color: Color, bg_color: Color) {
+    if let Some(title) = window.title.clone() {


this eagerly clones the whole title String every time. Instead move the .clone() operation into the inside of the branch

kevinaboos · 2022-12-29T01:59:05Z

+}
+
+pub fn print_string(
+    window: &mut MutexGuard<Window>,


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

kevinaboos · 2022-12-29T02:10:10Z

+}
+
+/// FrameBuffer with no actual physical memory mapped,
+/// used for Window and WindowManager's backbuffer.


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).

kevinaboos · 2022-12-29T02:27:04Z

+        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) {


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).

kevinaboos · 2022-12-29T02:28:45Z

+    fn draw_unchecked(&mut self, x: isize, y: isize, col: Color) {
+        let x = x;
+        let y = y;
+        unsafe {


definitely no need to use unsafe here or anywhere else in this entire crate.

kevinaboos · 2022-12-29T02:31:42Z

+    }
+
+    // We don't want user to draw on top a border
+    pub fn return_drawable_area(&self) -> Rect {


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()

m-hugo

and you have many methods and fields that should be pub but aren't
or you could remove every pub if you don't care about modularity

m-hugo · 2023-01-05T22:05:58Z

+        })
+    }
+
+    fn copy_windows_into_main_vbuffer(&mut self, window: &mut MutexGuard<Window>) {


this should really be a Window method

Suggested change

fn copy_windows_into_main_vbuffer(&mut self, window: &mut MutexGuard<Window>) {

fn copy_into_vbuffer(&mut self, vbuf: &mut VirtualFrameBuffer){

kevinaboos · 2023-01-09T04:37:46Z

+        }
+    }
+
+    fn draw_unchecked(&mut self, x: isize, y: isize, col: Color) {


remove unsafety

kevinaboos · 2023-01-24T19:30:50Z

Closing in favor of #823

kevinaboos requested changes Dec 12, 2022

View reviewed changes

kevinaboos marked this pull request as draft December 12, 2022 18:29

ouz-a force-pushed the pr-branch branch 2 times, most recently from f1ca63a to e5b5928 Compare December 14, 2022 12:56

kevinaboos requested changes Dec 29, 2022

View reviewed changes

ouz-a added 15 commits January 5, 2023 21:40

Initial commit

440006c

use is_supported

cdffd0b

z order, remove most of bounds check

c27911d

faster rendering border rendering and window titles

ec3656f

different types for virtual and physical buffers

45c0d0f

typeset Color = u32

03d423b

Use RelativePos for Window related functions

347dedd

use new types in appropriate places

8816e1c

only clone window title when needed

dc560da

fix window ref

d351b65

cache ubiquitous rectangles

84e4acc

windows shouldn't be smaller than 50x50

55bf2dc

make it readable

df21b36

introduce FrameBufferChunker

8ca8adf

make the core more readable

94f4af0

ouz-a force-pushed the pr-branch branch from d6cf7f0 to 94f4af0 Compare January 5, 2023 18:40

ouz-a added 4 commits January 5, 2023 21:45

use new way to get graphic info

2f2007c

fix window bounce add stride info

45ada86

some more clean up

ea2269a

clippy lints

6022562

m-hugo reviewed Jan 5, 2023

View reviewed changes

ouz-a added 4 commits January 7, 2023 00:54

fix a mouse bug

cec65e9

remove logs

965557e

fix window movement bug

c00e901

fix window resizing bug

53695c9

kevinaboos reviewed Jan 9, 2023

View reviewed changes

ouz-a added 9 commits January 9, 2023 13:14

remove every unwrap

02afd89

apps are strong again

8fd7106

use iterator to draw mouse

529f173

don't use unsafe when rendering text

8d53776

handle unwrap

6d29a72

refactor

6eccb69

fix window dragging and rendering order bug

7e92836

a little cleanup and a small fps counter app

ed74e20

no more unsafety

e09669b

ouz-a mentioned this pull request Jan 22, 2023

Replace old graphics api with Porthole #823

Draft

4 tasks

kevinaboos closed this Jan 24, 2023

	fn copy_windows_into_main_vbuffer(&mut self, window: &mut MutexGuard<Window>) {
	fn copy_into_vbuffer(&mut self, vbuf: &mut VirtualFrameBuffer){

Uh oh!

Conversation

ouz-a commented Dec 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Porthole

Design

Tasks:

Uh oh!

kevinaboos left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kevinaboos left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

m-hugo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kevinaboos commented Jan 24, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ouz-a commented Dec 9, 2022 •

edited

Loading