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: 233 split winit,gpu from core #290

Closed
wants to merge 11 commits into from

Conversation

zoechi
Copy link
Contributor

@zoechi zoechi commented Mar 15, 2023

fixes #233
follow-up to #273

This still needs cleanup.

I was thinking about the suggested ShellWindow,
but I'a quite lost on that topic.
Perhaps you can have a look and provide some more feedback this topic and also to the general direction in this PR.

I made an attempt at Traits for Application, WindowBuilder, EventLoop and thought about how to put this together into a ShellWindow somehow, but it didn't make too much sense to me.
These things only contain Winit-specific code and don't interact with the rest of the code. Creating some abstraction that would match the same structure for Winit and possible Winit-alternatives seems rather limiting instead of gaining any flexibilty or improved dev experience.
Perhaps you can shed some more light on that.

@codecov
Copy link

codecov bot commented Mar 15, 2023

Codecov Report

Merging #290 (1310d86) into master (a0665d2) will decrease coverage by 3.15%.
The diff coverage is 21.76%.

@@            Coverage Diff             @@
##           master     #290      +/-   ##
==========================================
- Coverage   72.30%   69.15%   -3.15%     
==========================================
  Files         161      193      +32     
  Lines       19032    20010     +978     
==========================================
+ Hits        13761    13838      +77     
- Misses       5271     6172     +901     
Impacted Files Coverage Δ
app/src/application.rs 0.00% <0.00%> (ø)
core/src/builtin_widgets/box_decoration.rs 66.48% <0.00%> (ø)
core/src/context/event_context.rs 66.66% <ø> (ø)
core/src/events.rs 71.42% <ø> (ø)
core/src/events/character.rs 89.28% <ø> (ø)
core/src/events/event.rs 0.00% <0.00%> (ø)
crossterm/src/from_device_id.rs 0.00% <0.00%> (ø)
crossterm/src/from_element_state.rs 0.00% <0.00%> (ø)
crossterm/src/from_event.rs 0.00% <0.00%> (ø)
crossterm/src/from_modifiers.rs 0.00% <0.00%> (ø)
... and 40 more

... and 2 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

let pos = position.to_logical::<f32>(wnd_factor);
self.cursor_move_to(Point::new(pos.x, pos.y), tree)
let logical_pos =
ScaleToLogic::new(1.0 / wnd_factor as f32).transform_point(position.cast());
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the core window does not need ScaleToLogic and window_factor, wnd_factor should transparent to the core window. The s
hell window converts all positions to the logic axis before passing it to the core window.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this mean the distinction between physical and logical should not exist at all within Ribir, but always be resolved in ribir_winit? Or did you mean just this place?
The distinction comes mostly from the part of ribir_painter that I moved out to ribir_geometry.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it should all resolve in ribir_winit, ribir_app need to know it to transform the painter.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Logic means scale, not axis, right?
ribir_painter used DeviceSize (physic)
I'm not sure how this can work. When everything is converted to logic before passed to the core window, does this need to converted to physics before passed to painter? That doesn't seem right to me.
Logic values use float. I'm not sure how that works with painting.
Can you please shed some light on that?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now, it's used as a scale. Logically axis means Ribir will support a Transform2D to convert the axis from the device, for example, embedding Ribir in another UI system, at least needing translation and scale.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this need to converted to physics before passed to painter?

You don't need to do it. When constructing a painter we pass a scale or Transform matrix to it, the painter will do it by itself.

@M-Adoo
Copy link
Collaborator

M-Adoo commented Mar 16, 2023

@zoechi I review it all over and looks great. Thanks!

  1. the core window abstract is what I expect and I leave a comment about the logic axis.
  2. You should not put Application, Eventloop, and render-backends to ShellWindow, ShellWindow is just a trait, provide an bridge ability between a platform window and a core window, its implementation should hold the core window.
  3. The Application is not a trait, it controls the shell windows, envetloop, and submits the paint commands to the painter-backend.
  4. After we abstract the core window and shell window we have chance to choose to use winit or winit-alternatives, and supporting the new platform is not so hard, eg. if we support embedded devices in the future. The abstract struct should not have the same struct as winit, but a more abstract one. For example, the core window does not have a mouse event, the pointer event will cover it. we can reduce more concepts in the core window in the future, and these concepts are what the Ribir user needs to face.

@zoechi
Copy link
Contributor Author

zoechi commented Mar 16, 2023

@M-Adoo Thanks for your feedback. I still have troubles seeing what the end result should look like.
I think the idea is to create an abstraction for what I have put into the ribir_winit crate, but I couldn't find anything there that is worth generalizing.
If we introduce a winit-alternative, a similar package as ribir_winit needs to be created for this alternative. There isn't anything that could be reused from ribir_winit.
There it's only very little code that needs to be changed in the application code when switching between winit and an alternative. Creating an abstraction wouldn't change that, because the initialization of the concrete implementation still has to be in the applicaiton code.

@M-Adoo
Copy link
Collaborator

M-Adoo commented Mar 16, 2023

If we abstract the window, we needn't change the application code, we can just use the feature config to switch ribr_winit, and the user has a chance to use self alternatives, with a shell window type for the application, like

Application::<MyShellWindow>::run()

Our target is not to reuse the code in ribir_winit, it's just to convert the platform window stuff to Ribir core. But provide the ability to painlessly switch the shell window. So the ribir_winit package is just an adapter package for the platform. Finally, Ribir isolates all platform dependencies in the shell window and painter backend. To run the test we can have a mock backend and mock shell window, on the server side we can have another painter backend and shell window.

@zoechi
Copy link
Contributor Author

zoechi commented Mar 16, 2023

Ok, I guess I understand now what you mean.

@zoechi
Copy link
Contributor Author

zoechi commented Mar 16, 2023

we can just use the feature config to switch ribr_winit

In which crate should this happen? The crate where this happens needs to have a dependency to ribir_winit and all used alternatives.
This can either be an ribir_app package (that I introduced but doesn't contain real code yet) or in the custom application.
If this is done in the custom application, then the ribir_app crate is not necessary at all. A trait in ribir_core would be enough.

@M-Adoo
Copy link
Collaborator

M-Adoo commented Mar 16, 2023

we can just use the feature config to switch ribr_winit

In which crate should this happen? The crate where this happens needs to have a dependency to ribir_winit and all used alternatives. This can either be an ribir_app package (that I introduced but doesn't contain real code yet) or in the custom application. If this is done in the custom application, then the ribir_app crate is not necessary at all. A trait in ribir_core would be enough.

In ribir_app, ribir_app is needed, we'll support multi-windows in it.

zoechi added 3 commits March 21, 2023 15:05
cd ribir
cargo run --example todo_mvp -F winit,wgpu_gl

or

cd ribir
cargo run --example todo_mvp -F crossterm
@zoechi
Copy link
Contributor Author

zoechi commented Mar 22, 2023

I'm still not sure about the direction for the ShellWindow API.
I also haven't figured out the role of the EventLoop yet and how it should be integrated.
Some feedback would be appreciated (didn't took time to look into it yet).

I added a dummy Crossterm backend implementation so that I have something concrete to develop towards.
Hint: When running examples using it, there is no way to exit the app except killing it from another terminal session.

@M-Adoo
Copy link
Collaborator

M-Adoo commented Mar 23, 2023

@zoechi For ShellWindow and EventLoop, they are almost always in pairs.

  • EventLoop directly control the application lifetime and plays the role of a bridge from the platform to the core window it processes platform events and passes them to the core window.
  • ShellWindow displays the core window status, so it needs APIs like resize, set_title, and so on to respond to the core window.

@zoechi
Copy link
Contributor Author

zoechi commented Mar 23, 2023

  • EventLoop directly control the application lifetime and plays the role of a bridge from the platform to the core window it processes platform events and passes them to the core window.

In the case of Winit, EventLoop seems to be provided. It's not clear to me where or how I should prepare an API for it.
How would application developers interact with it?

  • ShellWindow displays the core window status, so it needs APIs like resize, set_title, and so on to respond to the core window.

It appears to me core::window::Window would the right place for that.
Did you mean to rename it to ShellWindow? If not, I don't understand how this new ShellWindow should be integrated.

@M-Adoo
Copy link
Collaborator

M-Adoo commented Mar 23, 2023

It appears to me core::window::Window would the right place for that.
Did you mean to rename it to ShellWindow? If not, I don't understand how this new ShellWindow should be integrated.

No, core window have its window config and need to pass to shell window to display. I try to define some APIs for you and feel free to rename and adjust it.

pub trait Eventloop {
  type SW: ShellWindow;
  // start the event loop, dispatch the events to shell windows.
  fn exec(&mut self, app: &mut Application);
  fn stop(&mut self);
  fn exit(self);
}

Application or AppCtx own the Eventloop, so user can control the application lifetime across AppCtx, and Eventloop start with Application, so it can access all core window and ShellWindow, it uses ShellWindow to detect the target window of platform event, and convert the event to core window.

impl ribir_core::Window {
  pub fn resize(&mut self, size: Size) {
    if let Some(shell) = self.shell_wnd.as_ref() {
      shell.resize(size);
    }
  }
  pub fn shell_wnd(&self) -> Option<&dyn ShellWindow>  {
    
  }

  ...
}

pub trait ShellWindow {

  /// construct a shell window for core window.
  fn constructor(core_wnd: &core::Window) -> Self;
  fn resize(&mut self, size: Size);
  fn set_title(&mut self, size: Size);
  ... 
}

Core window own an Option<Box<dyn ShellWindow>>, so it always passes its config change to ShellWindow if a ShellWindow have.

@zoechi
Copy link
Contributor Author

zoechi commented Apr 13, 2023

I hope to be able to continue soon, but I'll close for now.

@zoechi zoechi closed this Apr 13, 2023
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