Inlyne-Project / inlyne

Introducing Inlyne, a GPU powered yet browserless tool to help you quickly view markdown files in the blink of an eye.
MIT License
1.09k stars 27 forks source link

Fonts can appear faint / low contrast #102

Open CosmicHorrorDev opened 1 year ago

CosmicHorrorDev commented 1 year ago

See https://github.com/trimental/inlyne/issues/97#issuecomment-1540796548

@nicoburns Spinning this off into it's own issues since it seems distinct

Reference image

image

It looks like we do set the background color based on the theme. I'm not sure what causes the colors to look fainter for you. This is what I get

image

which is much more readable.

Here's the HTML we're rendering for reference ```html
use taffy::prelude::*;

// First create an instance of Taffy
let mut taffy = Taffy::new();

// Create a tree of nodes using `taffy.new_leaf` and `taffy.new_with_children`.
// These functions both return a node id which can be used to refer to that node
// The Style struct is used to specify styling information
let header_node = taffy
    .new_leaf(
        Style {
            size: Size { width: length(800.0), height: length(100.0) },
            ..Default::default()
        },
    ).unwrap();

let body_node = taffy
    .new_leaf(
        Style {
            size: Size { width: length(800.0), height: auto() },
            flex_grow: 1.0,
            ..Default::default()
        },
    ).unwrap();

let root_node = taffy
    .new_with_children(
        Style {
            flex_direction: FlexDirection::Column,
            size: Size { width: length(800.0), height: length(600.0) },
            ..Default::default()
        },
        &[header_node, body_node],
    )
    .unwrap();

// Call compute_layout on the root of your tree to run the layout algorithm
taffy.compute_layout(root_node, Size::MAX_CONTENT).unwrap();

// Inspect the computed layout using taffy.layout
assert_eq!(taffy.layout(root_node).unwrap().size.width, 800.0);
assert_eq!(taffy.layout(root_node).unwrap().size.height, 600.0);
assert_eq!(taffy.layout(header_node).unwrap().size.width, 800.0);
assert_eq!(taffy.layout(header_node).unwrap().size.height, 100.0);
assert_eq!(taffy.layout(body_node).unwrap().size.width, 800.0);
assert_eq!(taffy.layout(body_node).unwrap().size.height, 500.0); // This value was not set explicitly, but was computed by Taffy

```

Notably I don't see anything that looks like it would impact the font width

Can you try setting a different font to see if it's only the font that is getting selected? You can do this by making a config file at ~/.config/inlyne/inlyne.toml or wherever that translates to for your system. You would just need

[font-options]
monospace-font = "{Insert desired font family}"
nicoburns commented 1 year ago

I'm not convinced it's just the background colour. The text itself seems to be pale. The colour used for let in your screenshot is #b58fae, whereas the colour github is using length is #8250df. These are very different!

Can you try setting a different font to see if it's only the font that is getting selected? You can do this by making a config file at ~/.config/inlyne/inlyne.toml or wherever that translates to for your system

Will try this tomorrow.

nicoburns commented 1 year ago

It could be a thickness thing. It does look like maybe some of the colour is getting anti-aliased away.

CosmicHorrorDev commented 1 year ago

I guess I wasn't very clear. I also don't think it's from the background color now

We do currently set a tolerance of 0.5 for the draw cache, so a lower value might lead to more accurate rendering of the glyphs. Although it doesn't really make sense that there would be such a universal impact

https://github.com/trimental/inlyne/blob/da76a06b6d68b74bf1e885bdce2e61587f07f3a8/src/renderer.rs#L157-L159

CosmicHorrorDev commented 1 year ago

@nicoburns can you confirm if this issue exists on current main? The switch to cosmic-text just got merged in today

nicoburns commented 1 year ago

The following is a screenshot of main compared with the screenshots in https://github.com/trimental/inlyne/issues/97#issuecomment-1540796548:

Screenshot 2023-05-19 at 02 49 57

To me it looks like the contrast issue has been fixed, although:

CosmicHorrorDev commented 1 year ago
  • main seems to no longer be rendering code blocks using a monospace font

Welp that's definitely a bug lol. It totally slipped by me in review since I set a custom monospace font in my inlyne.toml. I can dig into fixing that tomorrow

The theme that inlyne is using isn't quite as clear as the one github is using

Yeah the default light theme seems to be lower contrast for sure. These are all of the light themes we have bundled in from syntect. inspired-github seems to have much better contrast, but I don't like that it looks like they went with #ffffff for the background color although we could always forcibly override that. I believe that the inspired-github theme also uses different font-weights for some of the tokens which we haven't added support for yet, but that should be easy enough to handle

image

There's also always the possibility of us adding in a custom theme too

(Aside: Thanks for all of your help looking through these things :heart:. It's easy to miss issues after you've acclimated to them)

trimental commented 1 year ago

The problem is with this line here https://github.com/pop-os/cosmic-text/blob/bfb5eefbfa869915e47824877af68a5307cf301c/src/attrs.rs#L156. Seems as though it'll sacrifice all text attributes to use a font with emojis.

nicoburns commented 1 year ago

Hmm.... multiple fonts should match with that function though. There must be somewhere else ordering them.

trimental commented 1 year ago

I think I've got a fairly decent fix here https://github.com/trimental/cosmic-text/commit/e1e9fb5215daa9dbd40998e16210a6ee3a61ff1f.

trimental commented 1 year ago

It does seem like cosmic-text is lacking activity recently. It may be worth creating our own fork to apply fixes like this. Thoughts @CosmicHorrorDev.

CosmicHorrorDev commented 1 year ago

I'm AOK with patching with our own forks for now and trying to upstream stuff before a new inlyne release

Only real issue would be if they start heavily diverging from our forks, but that's unlikely

nicoburns commented 1 year ago

Cosmic-text has historically been reasonably good at accepting PRs. Just a bit bursty in terms of development. There is also this thread on the Iced Discord server (https://discord.com/channels/628993209984614400/1027584473631555604) which has become the sort of unnofficial support forum for cosmic-text.

And a couple of open issues around font-fallback behaviour:

I suspect a more general algorithm implementation (rather than something that just works for monospace fonts) might be more likely to be accepted upstream.

trimental commented 1 year ago

@nicoburns if possible could you test https://github.com/trimental/inlyne/pull/120 to affirm that monospace fallback is fixed?

nicoburns commented 1 year ago

Yep, that branch selects a monospace font for code sections. However it still looks quite weird. Two issues I would point to:

Screenshot 2023-05-20 at 14 41 16
trimental commented 1 year ago

Seems git has scuffed the commit names but both https://github.com/trimental/inlyne/commit/8b441006757b45a6f6c5175b04ec015ec7a8e4a3 and https://github.com/trimental/inlyne/commit/ef8d5202add3678846025d8500155df9ba8e1d78. Should enable code bolding and fix the line spaces respectively.

As for the washed out appearance I believe this is due to this. There's no real way for us to determine whether to do srgb conversion or not. Perhaps we should just set the default theme to InspiredGithub for now or manually create a table for which themes need conversion.

CosmicHorrorDev commented 1 year ago

Perhaps we should just set the default theme to InspiredGithub for now

Already working on that :+1:

or manually create a table for which themes need conversion.

We only have a handful of bundled themes, so that should be doable although it's possible that future updates could cause things to get out of sync with the hardcoded values

nicoburns commented 1 year ago

Seems git has scuffed the commit names but both https://github.com/trimental/inlyne/commit/8b441006757b45a6f6c5175b04ec015ec7a8e4a3 and https://github.com/trimental/inlyne/commit/ef8d5202add3678846025d8500155df9ba8e1d78. Should enable code bolding and fix the line spaces respectively.

Line spacing is fixed in that it is now even, but IMO the lines are now a little too close togther. A little spacing would be nice. However, I'm not seeing any bolding, and I'm still seeing weird glyph rendering where some characters and parts of the characters seem to be stroked more strongly than others:

Screenshot 2023-05-20 at 18 29 15

Perhaps this is just

As for the washed out appearance I believe this is due to this. There's no real way for us to determine whether to do srgb conversion or not. Perhaps we should just set the default theme to InspiredGithub for now or manually create a table for which themes need conversion.

That makes sense. An incorrect colour space is exactly what it looks like. I'd suggest that both of those fixes might be a good idea.

nicoburns commented 1 year ago

Update: I get better font rendering by switching the font. This is Source Sans Pro:

Screenshot 2023-05-20 at 18 43 15
nicoburns commented 1 year ago

Also works the other way with proportional fonts. Helvetica Neue doesn't look great:

Screenshot 2023-05-20 at 18 47 16
trimental commented 1 year ago

Bolding doesn't occur with Base16OceanLight. InspiredGithub has it though. image

felixsanz commented 1 year ago

in my case not only fonts, even images appear with wrong contrast and different colors than should be:

1694176554

1694176810

Above is inlyne, below is normal image opened with firefox

Any idea why this happens?

CosmicHorrorDev commented 8 months ago

There's a lot of potentially related discussion in this iced issue https://github.com/iced-rs/iced/issues/2254