Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DRAFT: Remove ribir_gpu and winit dependencies from ribir_core #273

Closed

Conversation

zoechi
Copy link
Contributor

@zoechi zoechi commented Mar 6, 2023

fixes #233
Move the dependencies to a new create ribir_app_winit and create abstractions to break up dependencies in ribir_core code.

There is still some cleanup necessary (naming, tests, ...) but I first wanted some feedback if this goes in the right direction before I put more work into it.

@M-Adoo
Copy link
Collaborator

M-Adoo commented Mar 6, 2023

@zoechi Thanks!

There are some points as what I expect, I think it in the right way.

  • add an application crate to control the windows
  • provide core::Cursor and core::Key to abstract the platform dependencies.

There are some points I'm concerned about:

  • We export too much core stuff.
  • All listeners remove to app_winit, these listeners are built-in widgets and should be part of the core.

From an overall perspective, we can make some adjustments:

  • We abstract events, layout, and draw logic in the core, so we provide a core window as the public struct to dispatch core events and draw frame. When we call draw_frame on the core window, it only returns the paint commands that do not depend on the render-backend.
  • We create an app package, which helps the application manages the multi-windows, control the event loop and mix the core window and the platform window, submit the paint commands of generate from the core window to a render-backend. It also can not depend on winit. It provides an abstract ShellWindow trait and EventLoop trait. The ShellWindow should convert all platform stuff to the core window need, like events, and cursor icons. The EventLoop provide convenient methods to control the application loop.
  • A wint shell window crate can be created, implementing the ShellWindow and EventLoop. And it should be not hard to be instead.

@zoechi
Copy link
Contributor Author

zoechi commented Mar 6, 2023

@M-Adoo Thanks for the feedback!
While I dug a bit into the code to find some line where to break up dependencies, I mostly did mechanical work to make the code compile after the changes.

I guess I'll need a while to process your comments and dig deeper into the code for better understanding what this all means exactly.
I'll come back with questions later.

@zoechi
Copy link
Contributor Author

zoechi commented Mar 9, 2023

  • We export too much core stuff.
  • All listeners remove to app_winit, these listeners are built-in widgets and should be part of the core.

Do you mean with this, that the src/event_dispatcher.rs code should be moved back from app_winit to core and instead the winit *Event stuff be mapped to core events?

@zoechi
Copy link
Contributor Author

zoechi commented Mar 9, 2023

I tried to visualize how I interpreted your comments, but it didn't work out too well. I'll post it here anyway

---
title: Architecture
---

graph TD;
  subgraph ribir_core
  core::Event["core::Event (Window/Device/User)"]
  core::Cursor
  core::Key
  core::Layout
  core::Window
  end

  subgraph ribir_app
  app::ShellWindow --> core::Window
  end

  subgraph winit
  winit::Event["winit::Event (Window/Device/User)"]
  winit::Cursor
  winit::Key
  winit::Window
  end

  subgraph ribir_winit
  ribir_winit::FromEvent -..-> winit::Event
  ribir_winit::FromEvent -..-> core::Event
  ribir_winit::FromCursor -..-> winit::Cursor 
  ribir_winit::FromCursor -..-> core::Cursor
  ribir_winit::FromKey -..-> winit::Key 
  ribir_winit::FromKey -..-> core::Key
  ribir_winit::FromWindow -..-> winit::Window
  ribir_winit::FromWindow -..-> core::Window
  end

  subgraph ribir_gpu
  gpu::Backend
  end

  subgraph custom_app
  custom::App -..-> winit::Window
  custom::App -..-> gpu::Backend
  custom::App -..-> app::ShellWindow
  end
Loading

So the end result is, that a custom_app project that initializes a ribir_app::ShellWindow with ribir_winit and ribir_gpu

@zoechi
Copy link
Contributor Author

zoechi commented Mar 9, 2023

Do you think it adding a ribir_geometry package that contains the euclid stuff from ribir_painter/lib. Currently size, position, ... are used from winit::dpi, but that is supposed to be removed from ribir_core.

@M-Adoo
Copy link
Collaborator

M-Adoo commented Mar 9, 2023

  • We export too much core stuff.
  • All listeners remove to app_winit, these listeners are built-in widgets and should be part of the core.

Do you mean with this, that the src/event_dispatcher.rs code should be moved back from app_winit to core and instead the winit *Event stuff be mapped to core events?

Yes.

@M-Adoo
Copy link
Collaborator

M-Adoo commented Mar 9, 2023

I tried to visualize how I interpreted your comments, but it didn't work out too well. I'll post it here anyway

So the end result is, that a custom_app project that initializes a ribir_app::ShellWindow with ribir_winit and ribir_gpu

Just a little bit different, I don't think the user needs to implement a custom_app, that ribir_app's works. ribir_app provide a feature config to the user to disable ribir_winit and can use another one instead of ribir_winit.

@M-Adoo
Copy link
Collaborator

M-Adoo commented Mar 9, 2023

Do you think it adding a ribir_geometry package that contains the euclid stuff from ribir_painter/lib. Currently size, position, ... are used from winit::dpi, but that is supposed to be removed from ribir_core.

euclid looks fine to me, and the size position... all described in a logic axis, we remove winit::dpi will not affect them. But ribir_winit should always convert the axis from winit axis to core logic axis by the winit::dpi.

@zoechi
Copy link
Contributor Author

zoechi commented Mar 9, 2023

I don't think the user needs to implement a custom_app

This is just supposed to be the project Ribir is used in to build the GUI

Thanks for your comments. I think all is clear now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ribir_core should not depends ribir_gpu and winit
2 participants