Skip to content

Commit

Permalink
Font artifact fix
Browse files Browse the repository at this point in the history
I noticed some random artifacts when drawing some text at certain sizes.
It was hard to predict when it would happen, and ultimately I discovered
that when glphys that where located on the atlas next to each other were
sampled, the neighboring glyph would bleed over. Many glphys have space
on all sides, which makes the issue pretty minimal.

Ultimately I discovered that this really only affected multisampling. I
couldn't figure out a way to enforce that MSAA clamps at arbitrary
locations, so instead I've made the TextureCollection automatically
insert padding when multisampling is enabled.

The coordinates being provided to the shader were whole number pixel
coordinates. I do not think this was a problem of bad data going into
the shader.
  • Loading branch information
ecton committed Nov 30, 2024
1 parent 586c0df commit db5a474
Show file tree
Hide file tree
Showing 6 changed files with 55 additions and 26 deletions.
5 changes: 5 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
the underlying render pass.
- `Text::align` allows specifying a `cosmic_text::Align` setting and a width to
perform the alignment within.
- `TextureCollection` now automatically adds padding when multisampling is
enabled to ensure that multisampling does not sample neighboring pixels of
other textures in the collection.
- `Drawing::draw_measured_text` now uses the correct offset when using
`TextOrigin::FirstBaseline`.

### Fixed

Expand Down
22 changes: 16 additions & 6 deletions src/atlas.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,8 @@ use std::sync::{Arc, PoisonError, RwLock};

use alot::{LotId, Lots};
use etagere::{Allocation, BucketedAtlasAllocator};
use figures::units::UPx;
use figures::{IntoSigned, IntoUnsigned, Point, Px2D, Rect, Size, UPx2D};
use figures::units::{Px, UPx};
use figures::{IntoSigned, IntoUnsigned, Point, Px2D, Rect, Size, UPx2D, Zero};

use crate::pipeline::{PreparedGraphic, Vertex};
use crate::{sealed, CanRenderTo, Graphics, Kludgine, KludgineGraphics, Texture, TextureSource};
Expand Down Expand Up @@ -38,6 +38,7 @@ struct Data {
rects: BucketedAtlasAllocator,
texture: Texture,
textures: Lots<Allocation>,
padding: Px,
}

impl TextureCollection {
Expand All @@ -56,6 +57,12 @@ impl TextureCollection {
filter_mode,
);

let padding = if graphics.multisample_state().count > 1 {
Px::new(1)
} else {
Px::ZERO
};

let initial_size = initial_size.into_signed();
Self {
format,
Expand All @@ -67,6 +74,7 @@ impl TextureCollection {
)),
texture,
textures: Lots::new(),
padding,
})),
}
}
Expand Down Expand Up @@ -107,11 +115,11 @@ impl TextureCollection {
graphics: &impl KludgineGraphics,
) -> CollectedTexture {
let mut this = self.data.write().unwrap_or_else(PoisonError::into_inner);
let signed_size = size.into_signed();
let allocation_size = size.into_signed() + Size::squared(this.padding * 2);
let allocation = loop {
if let Some(allocation) = this.rects.allocate(etagere::euclid::Size2D::new(
signed_size.width.into(),
signed_size.height.into(),
allocation_size.width.get(),
allocation_size.height.get(),
)) {
break allocation;
}
Expand Down Expand Up @@ -143,7 +151,9 @@ impl TextureCollection {
};

let region = Rect::new(
Point::px(allocation.rectangle.min.x, allocation.rectangle.min.y).into_unsigned(),
(Point::px(allocation.rectangle.min.x, allocation.rectangle.min.y)
+ Point::squared(this.padding))
.into_unsigned(),
size,
);

Expand Down
2 changes: 1 addition & 1 deletion src/drawing.rs
Original file line number Diff line number Diff line change
Expand Up @@ -465,7 +465,7 @@ mod text {
(Point::from(text.source.size).into_px(scaling_factor) / 2).round()
}
TextOrigin::FirstBaseline => {
Point::new(Px::ZERO, text.source.ascent.into_px(scaling_factor))
Point::new(Px::ZERO, text.source.line_height.into_px(scaling_factor))
}
TextOrigin::Custom(offset) => offset.into_px(scaling_factor),
};
Expand Down
11 changes: 11 additions & 0 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,7 @@ impl Kludgine {
linear_sampler: &linear_sampler,
nearest_sampler: &nearest_sampler,
uniforms: &uniforms.wgpu,
multisample,
}),
default_bindings,
pipeline,
Expand Down Expand Up @@ -447,6 +448,7 @@ struct ProtoGraphics<'gfx> {
linear_sampler: &'gfx wgpu::Sampler,
nearest_sampler: &'gfx wgpu::Sampler,
uniforms: &'gfx wgpu::Buffer,
multisample: wgpu::MultisampleState,
}

impl<'a> ProtoGraphics<'a> {
Expand All @@ -459,6 +461,7 @@ impl<'a> ProtoGraphics<'a> {
linear_sampler: &kludgine.linear_sampler,
nearest_sampler: &kludgine.nearest_sampler,
uniforms: &kludgine.uniforms.wgpu,
multisample: kludgine.multisample_state(),
}
}
}
Expand Down Expand Up @@ -493,6 +496,10 @@ impl sealed::KludgineGraphics for ProtoGraphics<'_> {
fn linear_sampler(&self) -> &wgpu::Sampler {
self.linear_sampler
}

fn multisample_state(&self) -> wgpu::MultisampleState {
self.multisample
}
}

impl KludgineGraphics for Graphics<'_> {}
Expand Down Expand Up @@ -525,6 +532,10 @@ impl sealed::KludgineGraphics for Graphics<'_> {
fn linear_sampler(&self) -> &wgpu::Sampler {
&self.kludgine.linear_sampler
}

fn multisample_state(&self) -> wgpu::MultisampleState {
self.multisample
}
}

#[derive(Debug)]
Expand Down
1 change: 1 addition & 0 deletions src/sealed.rs
Original file line number Diff line number Diff line change
Expand Up @@ -104,4 +104,5 @@ pub trait KludgineGraphics {
fn uniforms(&self) -> &wgpu::Buffer;
fn nearest_sampler(&self) -> &wgpu::Sampler;
fn linear_sampler(&self) -> &wgpu::Sampler;
fn multisample_state(&self) -> wgpu::MultisampleState;
}
40 changes: 21 additions & 19 deletions src/text.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,9 @@ use std::sync::{Arc, Mutex, PoisonError, Weak};

use cosmic_text::{Align, Attrs, AttrsOwned, LayoutGlyph, SwashContent};
use figures::units::{Lp, Px, UPx};
use figures::{FloatConversion, Fraction, Point, Rect, Round, ScreenScale, Size, UPx2D, Zero};
use figures::{
FloatConversion, Fraction, IntoSigned, Point, Rect, Round, ScreenScale, Size, UPx2D, Zero,
};
use intentional::Cast;
use smallvec::SmallVec;

Expand Down Expand Up @@ -470,7 +472,7 @@ pub(crate) fn map_each_glyph(
bytes_per_row: Some(image.placement.width),
rows_per_image: None,
},
Size::upx(image.placement.width, image.placement.height).cast(),
Size::upx(image.placement.width, image.placement.height),
&ProtoGraphics {
id: kludgine.id,
device,
Expand All @@ -479,6 +481,7 @@ pub(crate) fn map_each_glyph(
linear_sampler: &kludgine.linear_sampler,
nearest_sampler: &kludgine.nearest_sampler,
uniforms: &kludgine.uniforms.wgpu,
multisample: kludgine.multisample,
},
),
true,
Expand All @@ -494,7 +497,7 @@ pub(crate) fn map_each_glyph(
bytes_per_row: Some(image.placement.width * 4),
rows_per_image: None,
},
Size::upx(image.placement.width, image.placement.height).cast(),
Size::upx(image.placement.width, image.placement.height),
&ProtoGraphics {
id: kludgine.id,
device,
Expand All @@ -503,6 +506,7 @@ pub(crate) fn map_each_glyph(
linear_sampler: &kludgine.linear_sampler,
nearest_sampler: &kludgine.nearest_sampler,
uniforms: &kludgine.uniforms.wgpu,
multisample: kludgine.multisample,
},
),
false,
Expand All @@ -523,16 +527,14 @@ pub(crate) fn map_each_glyph(
Rect::new(
(Point::new(physical.x, physical.y)).cast::<Px>()
+ Point::new(
image.placement.left,
metrics.line_height.cast::<i32>() - image.placement.top,
Px::new(image.placement.left),
Px::from(metrics.line_height) - image.placement.top,
),
Size::new(
i32::try_from(image.placement.width)
.expect("width out of range of i32"),
i32::try_from(image.placement.height)
.expect("height out of range of i32"),
UPx::new(image.placement.width),
UPx::new(image.placement.height),
)
.cast(),
.into_signed(),
),
color,
),
Expand Down Expand Up @@ -607,9 +609,11 @@ where
// TODO the returned type should be able to be drawn, so that we don't have to call update_scratch_buffer again.
let line_height = Unit::from_lp(kludgine.text.line_height, kludgine.effective_scale);
let mut min = Point::new(Px::MAX, Px::MAX);
let mut first_line_max_y = Px::MIN;
let mut last_baseline = Px::MIN;
let mut max = Point::new(Px::MIN, Px::MIN);
let mut ascent = Px::ZERO;
let mut descent = Px::ZERO;
let mut first_baseline = Px::ZERO;
let mut measured_glyphs = Vec::new();
map_each_glyph(
buffer,
Expand All @@ -625,7 +629,9 @@ where
max.x = max.x.max(line_width);
max.y = max.y.max(blit.bottom_right(baseline).y);
if line_index == 0 {
first_line_max_y = first_line_max_y.max(blit.bottom_right(baseline).y);
first_baseline = baseline;
ascent = ascent.max(baseline - blit.top_left().y);
descent = descent.min(baseline - blit.bottom_right(baseline).y);
}
if COLLECT_GLYPHS {
measured_glyphs.push(MeasuredGlyph {
Expand All @@ -646,20 +652,16 @@ where
glyphs: Vec::new(),
}
} else {
if first_line_max_y == Px::MIN {
first_line_max_y = line_height.into_px(kludgine.effective_scale);
}

MeasuredText {
ascent: line_height - Unit::from_px(min.y, kludgine.effective_scale),
descent: line_height - Unit::from_px(first_line_max_y, kludgine.effective_scale),
ascent: Unit::from_px(ascent, kludgine.effective_scale),
descent: Unit::from_px(descent, kludgine.effective_scale),
left: Unit::from_px(min.x, kludgine.effective_scale),
size: Size {
width: Unit::from_px(max.x, kludgine.effective_scale),
height: Unit::from_px(max.y.max(last_baseline), kludgine.effective_scale)
.max(line_height),
},
line_height,
line_height: Unit::from_px(first_baseline, kludgine.effective_scale),
glyphs: measured_glyphs,
}
}
Expand Down

0 comments on commit db5a474

Please sign in to comment.