Conversation
|
Awesome! The main problem I have with the implementation is the way that blending spreads through buffers. IMO a widget that expects or needs blending on a per buffer basis can use a wrapper (that we can provide If you don't want to deal with it but like the idea, we can merge this as is and refactor it later. <3 I will make a review in the form of comments for the smaller less important stuff that can we address later in some other PR. |
| /// | ||
| /// Returns the number of characters that were written. | ||
| pub fn draw_string<Buf: Buffer>(buf: &mut Buf, start: Position, s: &str) -> u16 { | ||
| pub fn draw_string<Buf: Buffer>(buf: &mut Buf, start: Position, string: &str, style: Style) -> u16 { |
There was a problem hiding this comment.
I was thinking about making a distinction between purely setting characters and setting characters and their style.
draw_text would have a style and string argument. Then it writes the string, setting the style for each cell written to.
draw_string only writes the string without touching the style.
This way its easier to tell if a style is being changed at a quick glance.
The change for this is fairly simple so if you want we can do it later or add it in this PR.
| for (x, ch) in (start.x..sz.width).zip(string.chars()) { | ||
| written += 1; | ||
| buf.get_cell_mut(Position { x, ..start }).ch = ch; | ||
| let cell = Cell { |
There was a problem hiding this comment.
The buffer way of implementing blending I mentioned would solve the difference between set and get_mut here as the caller explicitly states what they want.
| fn zero_area_buffer_returns_zero() { | ||
| let mut buf = make_buf(Size::ZERO); | ||
| let written = draw_string(&mut buf, Position::ZERO, "hello"); | ||
| let written = draw_string(&mut buf, Position::ZERO, "hello", Style::default()); |
There was a problem hiding this comment.
Not really a problem here but as a new user seeing this I would have two thoughts.
- Does this set the style to the default (clears)?
- Does this not touch the style at all?
The draw_text and draw_string make it more obvious.
germterm/src/core/mod.rs
Outdated
|
|
||
| let should_exit = update(self); | ||
|
|
||
| // At this point we already have pre-composed layers, |
There was a problem hiding this comment.
Blending or compositor specific don't really have a reason to be in the engine. They can be implemented in the form of a buffer (or a compositor trait?)
| .set_attributes(other.attributes() | self.attributes()); | ||
| } | ||
|
|
||
| /// Premultiplies the `fg` and `bg` RGB channels by alpha if applicable. |
There was a problem hiding this comment.
The compositor shouldn't really touch Style.
This method should be a function that takes two Color arguments and returns one of them. Nothing here is style specific.
Edit: Perhaps we move colors.rs to colors/mod.rs and collect the blending logic in its own module?
2 participants
The compositor at the time of writing this is made up of 2 simple functions,
compose_cell()andcompose_buffers(), withcompose_buffersdirectly calling intocompose_cell.To achieve layer support I've for the time being settled on a
BTreeMap<isize, FlatBuffer>field in the buffers, which usesFlatBufferas layers. This may or may not be the best solution in the long term, but we're rolling with it for now.The first composing step happens in
FlatBuffer::set_cell_checked(), where we compose the buffer in a JIT fashion. This is achieved by calling thecompose_cell()function, which mutates the bottom cell.The composed layers in the form of
FlatBuffers are then composed together into a single frame buffer at the end of the frame using thecompose_buffers()function.One thing to keep in mind is the need to premultiply the rgb channels by alpha for each and every color that "enters" the layers. In practice this means calling
Style::premultiply_fg_and_bg()on the cell which we're about to draw inFlatBuffer::set_cell_checked(). It's important we don't premultiply anything on the second layer-to-layer composing step, as the layer colors already had premultiplication applied by the previous step.@airblast-dev I'd appreciate your feedback here! I went for a more procedural approach with the top level functions, but we can move to a
Compositortrait if it'd lead to a nicer API or more possibilities.