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.

Sign up for GitHub

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

fix issue 158 Shading is computed wrong. #160

Merged
merged 8 commits into from
Dec 4, 2024
Merged

fix issue 158 Shading is computed wrong. #160

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
b5abbac
fix issue 158 Shading is computed wrong.
tomlm Nov 30, 2024
0d1c24e
Automated JetBrains cleanup
github-actions[bot] Nov 30, 2024
20f03da
Merge branch 'main' into tomlm/issue158
tomlm Dec 2, 2024
c8caa7a
Merge branch 'main' into tomlm/issue158
tomlm Dec 4, 2024
f523d00
* Changed Shaded pixel blending with to dim the original pixel then b…
tomlm Dec 4, 2024
0084cce
Merge branch 'tomlm/issue158' of https://github.com/jinek/Consolonia …
tomlm Dec 4, 2024
1ec065f
using
tomlm Dec 4, 2024
316fed5
Automated JetBrains cleanup
github-actions[bot] Dec 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion src/Consolonia.Core/Drawing/PixelBufferImplementation/Pixel.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@

public PixelBackground Background { get; init; }

public bool IsCaret { get; init; }

Check notice on line 85 in src/Consolonia.Core/Drawing/PixelBufferImplementation/Pixel.cs

View workflow job for this annotation

GitHub Actions / build 

"[AutoPropertyCanBeMadeGetOnly.Global] Auto-property can be made get-only" on /home/runner/work/Consolonia/Consolonia/src/Consolonia.Core/Drawing/PixelBufferImplementation/Pixel.cs(85,36)

[JsonIgnore] public ushort Width => Foreground.Symbol.Width;

Expand Down Expand Up @@ -125,9 +125,15 @@
newBackground = Background;
break;
case PixelBackgroundMode.Shaded:
// shade the current pixel
(newForeground, newBackground) = Shade();

// blend the pixelAbove foreground into the shaded pixel
newForeground = newForeground.Blend(pixelAbove.Foreground);
break;

// resulting in new pixel with shaded background and blended foreground
return new Pixel(newForeground, newBackground);
Copy link
Owner

Choose a reason for hiding this comment

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

Lets discuss. We can still draw a non blank symbol on top of the shade, right?

Copy link
Collaborator Author

@tomlm tomlm Dec 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think so. Am I losing something by converting the shade background to a transform on the existing color at this point?

Are you thinking we need to do be able to do something like this:
Pixel( Foreground('X', Colors.White), Background(Shaded))) ?

Currently I am doing this:
Shaded => Dim() existing pixel (foreground and background).

Do you think I should do:
Shaded => Dim() existing current pixel background, then blend current foreground, then I suppose Dim() the result?

Or actually put the Shaded background into the pixelbuffer and Dim() on render?

Copy link
Owner

Choose a reason for hiding this comment

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

Almost

Do you think I should do:
Shaded => Dim() existing current pixel background, then blend current foreground, then I suppose Dim() the result?

I'm thinking we should dim the foreground first, then just blend with foreground of new pixel.
This way, if there is no foreground above then foreground of old pixel is just dimmed. But if there is a new pixel, then it should be preserved and not shaded.

Copy link
Owner

Choose a reason for hiding this comment

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

I just understood, that before it was probably fine. But now we have different meaning of Blend procedure. I think it's correct you are getting rid of it here, but we just need to put correct symbol on top if there is one.


default: throw new ArgumentOutOfRangeException(nameof(pixelAbove));
}

Expand Down
33 changes: 18 additions & 15 deletions src/Consolonia.Core/Drawing/PixelBufferImplementation/PixelForeground.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
using System;
using System.ComponentModel;
using System.Diagnostics;
using System.Diagnostics.CodeAnalysis;
using Avalonia.Media;
Expand All @@ -14,13 +13,13 @@ public PixelForeground()
{
Symbol = new SimpleSymbol(" ");
Color = Colors.Transparent;
Weight = FontWeight.Normal;
Style = FontStyle.Normal;
Weight = null;
Style = null;
TextDecoration = null;
}

public PixelForeground(ISymbol symbol, Color color,
FontWeight weight = FontWeight.Normal, FontStyle style = FontStyle.Normal,
FontWeight? weight = null, FontStyle? style = null,
TextDecorationLocation? textDecoration = null)
{
ArgumentNullException.ThrowIfNull(symbol);
Expand All @@ -36,15 +35,13 @@ public PixelForeground(ISymbol symbol, Color color,
[JsonConverter(typeof(ColorConverter))]
public Color Color { get; init; }

[DefaultValue(FontWeight.Normal)]
[JsonProperty(DefaultValueHandling = DefaultValueHandling.IgnoreAndPopulate)]
public FontWeight Weight { get; init; }
[JsonProperty(NullValueHandling = NullValueHandling.Ignore)]
public FontWeight? Weight { get; init; }

[DefaultValue(FontStyle.Normal)]
[JsonProperty(DefaultValueHandling = DefaultValueHandling.Ignore)]
public FontStyle Style { get; init; }
[JsonProperty(NullValueHandling = NullValueHandling.Ignore)]
public FontStyle? Style { get; init; }

[JsonProperty(DefaultValueHandling = DefaultValueHandling.Ignore)]
[JsonProperty(NullValueHandling = NullValueHandling.Ignore)]
public TextDecorationLocation? TextDecoration { get; init; }

public bool Equals(PixelForeground other)
Expand All @@ -68,10 +65,15 @@ public PixelForeground Blend(PixelForeground pixelAboveForeground)
ISymbol symbolAbove = pixelAboveForeground.Symbol;
ArgumentNullException.ThrowIfNull(symbolAbove);

ISymbol newSymbol = Symbol.Blend(ref symbolAbove);
if (pixelAboveForeground.Color == Colors.Transparent)
// if pixelAbove is transparent then the foreground below should be unchanged.
return this;

return new PixelForeground(newSymbol, pixelAboveForeground.Color, pixelAboveForeground.Weight,
pixelAboveForeground.Style, pixelAboveForeground.TextDecoration);
return new PixelForeground(Symbol.Blend(ref symbolAbove),
pixelAboveForeground.Color,
pixelAboveForeground.Weight ?? Weight,
pixelAboveForeground.Style ?? Style,
pixelAboveForeground.TextDecoration ?? TextDecoration);
}

public override bool Equals([NotNullWhen(true)] object obj)
Expand All @@ -81,7 +83,8 @@ public override bool Equals([NotNullWhen(true)] object obj)

public override int GetHashCode()
{
return HashCode.Combine(Symbol, Color, (int)Weight, (int)Style, TextDecoration);
return HashCode.Combine(Symbol, Color, (int)(Weight ?? FontWeight.Normal), (int)(Style ?? FontStyle.Normal),
TextDecoration);
}

public static bool operator ==(PixelForeground left, PixelForeground right)
Expand Down
4 changes: 2 additions & 2 deletions src/Consolonia.Core/Drawing/RenderTarget.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,8 +156,8 @@ private struct FlushingBuffer
private readonly StringBuilder _stringBuilder;
private Color _lastBackgroundColor;
private Color _lastForegroundColor;
private FontStyle _lastStyle = FontStyle.Normal;
private FontWeight _lastWeight = FontWeight.Normal;
private FontStyle? _lastStyle;
private FontWeight? _lastWeight;
private TextDecorationLocation? _lastTextDecoration;
private PixelBufferCoordinate _currentBufferPoint;
private PixelBufferCoordinate _lastBufferPointStart;
Expand Down
4 changes: 2 additions & 2 deletions src/Consolonia.Core/Dummy/DummyConsole.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@
Size = new PixelBufferSize(width, height);
}

public PixelBufferSize Size { get; set; }

Check notice on line 27 in src/Consolonia.Core/Dummy/DummyConsole.cs

View workflow job for this annotation

GitHub Actions / build 

"[AutoPropertyCanBeMadeGetOnly.Global] Auto-property can be made get-only" on /home/runner/work/Consolonia/Consolonia/src/Consolonia.Core/Dummy/DummyConsole.cs(27,44)

public bool CaretVisible
{
Expand Down Expand Up @@ -54,8 +54,8 @@
{
}

public void Print(PixelBufferCoordinate bufferPoint, Color background, Color foreground, FontStyle style,
FontWeight weight, TextDecorationLocation? textDecoration, string str)
public void Print(PixelBufferCoordinate bufferPoint, Color background, Color foreground, FontStyle? style,
FontWeight? weight, TextDecorationLocation? textDecoration, string str)
{
}

Expand Down
4 changes: 2 additions & 2 deletions src/Consolonia.Core/Infrastructure/IConsole.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,8 @@ public interface IConsole : IDisposable
void SetCaretPosition(PixelBufferCoordinate bufferPoint);
PixelBufferCoordinate GetCaretPosition();

void Print(PixelBufferCoordinate bufferPoint, Color background, Color foreground, FontStyle style,
FontWeight weight, TextDecorationLocation? textDecoration, string str);
void Print(PixelBufferCoordinate bufferPoint, Color background, Color foreground, FontStyle? style,
FontWeight? weight, TextDecorationLocation? textDecoration, string str);

void WriteText(string str);

Expand Down
4 changes: 2 additions & 2 deletions src/Consolonia.Core/Infrastructure/InputLessDefaultNetConsole.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,8 +97,8 @@ public PixelBufferCoordinate GetCaretPosition()
return _headBufferPoint;
}

public void Print(PixelBufferCoordinate bufferPoint, Color background, Color foreground, FontStyle style,
FontWeight weight, TextDecorationLocation? textDecoration, string str)
public void Print(PixelBufferCoordinate bufferPoint, Color background, Color foreground, FontStyle? style,
FontWeight? weight, TextDecorationLocation? textDecoration, string str)
tomlm marked this conversation as resolved.
Show resolved Hide resolved
{
PauseTask?.Wait();
SetCaretPosition(bufferPoint);
Expand Down
8 changes: 4 additions & 4 deletions src/Consolonia.Designer/ConsolePreview.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -373,9 +373,9 @@ private class TextBlockComposer
private readonly StringBuilder _textBuilder;
private Color _lastBackgroundColor;
private Color _lastForegroundColor;
private FontStyle _lastStyle = FontStyle.Normal;
private FontStyle? _lastStyle;
private TextDecorationLocation? _lastTextDecorations;
private FontWeight _lastWeight = FontWeight.Normal;
private FontWeight? _lastWeight;
private double _textRunCharWidth;

public TextBlockComposer(StackPanel panel, double charWidth)
Expand Down Expand Up @@ -433,8 +433,8 @@ public void Flush()
Text = text,
Foreground = new SolidColorBrush(_lastForegroundColor),
Background = new SolidColorBrush(_lastBackgroundColor),
FontWeight = _lastWeight,
FontStyle = _lastStyle,
FontWeight = _lastWeight ?? FontWeight.Normal,
FontStyle = _lastStyle ?? FontStyle.Normal,
FontSize = 17,
TextDecorations = _lastTextDecorations switch
{
Expand Down
4 changes: 2 additions & 2 deletions src/Consolonia.NUnit/UnitTestConsole.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,8 +55,8 @@ PixelBufferCoordinate IConsole.GetCaretPosition()
return _fakeCaretPosition;
}

void IConsole.Print(PixelBufferCoordinate bufferPoint, Color background, Color foreground, FontStyle style,
FontWeight weight, TextDecorationLocation? textDecoration, string str)
void IConsole.Print(PixelBufferCoordinate bufferPoint, Color background, Color foreground, FontStyle? style,
FontWeight? weight, TextDecorationLocation? textDecoration, string str)
{
(ushort x, ushort y) = bufferPoint;

Expand Down
30 changes: 15 additions & 15 deletions src/Tests/Consolonia.Core.Tests/PixelForegroundTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,9 @@ public void Constructor()
Assert.That(pixelForeground.Color, Is.EqualTo(Colors.Transparent));
Assert.That(pixelForeground.Symbol.Text, Is.EqualTo(" "));
Assert.That(pixelForeground.Symbol.Width, Is.EqualTo(1));
Assert.That(pixelForeground.Weight, Is.EqualTo(FontWeight.Normal));
Assert.That(pixelForeground.Style, Is.EqualTo(FontStyle.Normal));
Assert.That(pixelForeground.TextDecoration, Is.Null);
Assert.IsNull(pixelForeground.Weight);
Assert.IsNull(pixelForeground.Style);
Assert.IsNull(pixelForeground.TextDecoration);
}

[Test]
Expand All @@ -29,9 +29,9 @@ public void ConstructorWithSymbol()
var pixelForeground = new PixelForeground(symbol, Colors.Red);
Assert.That(pixelForeground.Color, Is.EqualTo(Colors.Red));
Assert.That(pixelForeground.Symbol.Text, Is.EqualTo("a"));
Assert.That(pixelForeground.Weight, Is.EqualTo(FontWeight.Normal));
Assert.That(pixelForeground.Style, Is.EqualTo(FontStyle.Normal));
Assert.That(pixelForeground.TextDecoration, Is.Null);
Assert.IsNull(pixelForeground.Weight);
Assert.IsNull(pixelForeground.Style);
Assert.IsNull(pixelForeground.TextDecoration);
}

[Test]
Expand All @@ -42,8 +42,8 @@ public void ConstructorWithSymbolAndWeight()
Assert.That(pixelForeground.Color, Is.EqualTo(Colors.Red));
Assert.That(pixelForeground.Symbol.Text, Is.EqualTo("a"));
Assert.That(pixelForeground.Weight, Is.EqualTo(FontWeight.Bold));
Assert.That(pixelForeground.Style, Is.EqualTo(FontStyle.Normal));
Assert.That(pixelForeground.TextDecoration, Is.Null);
Assert.IsNull(pixelForeground.Style);
Assert.IsNull(pixelForeground.TextDecoration);
}

[Test]
Expand All @@ -53,9 +53,9 @@ public void ConstructorWithSymbolAndStyle()
var pixelForeground = new PixelForeground(symbol, Colors.Red, style: FontStyle.Italic);
Assert.That(pixelForeground.Color, Is.EqualTo(Colors.Red));
Assert.That(pixelForeground.Symbol.Text, Is.EqualTo("a"));
Assert.That(pixelForeground.Weight, Is.EqualTo(FontWeight.Normal));
Assert.IsNull(pixelForeground.Weight);
Assert.That(pixelForeground.Style, Is.EqualTo(FontStyle.Italic));
Assert.That(pixelForeground.TextDecoration, Is.Null);
Assert.IsNull(pixelForeground.TextDecoration);
}

[Test]
Expand All @@ -66,8 +66,8 @@ public void ConstructorWithSymbolAndTextDecorations()
var pixelForeground = new PixelForeground(symbol, Colors.Red, textDecoration: textDecoration);
Assert.That(pixelForeground.Color, Is.EqualTo(Colors.Red));
Assert.That(pixelForeground.Symbol.Text, Is.EqualTo("a"));
Assert.That(pixelForeground.Weight, Is.EqualTo(FontWeight.Normal));
Assert.That(pixelForeground.Style, Is.EqualTo(FontStyle.Normal));
Assert.IsNull(pixelForeground.Weight);
Assert.IsNull(pixelForeground.Style);
Assert.That(pixelForeground.TextDecoration, Is.EqualTo(TextDecorationLocation.Underline));
}

Expand All @@ -79,9 +79,9 @@ public void ConstructorWithWideCharacter()
var pixelForeground = new PixelForeground(symbol, Colors.Red);
Assert.That(pixelForeground.Color, Is.EqualTo(Colors.Red));
Assert.That(pixelForeground.Symbol.Text, Is.EqualTo("🎵"));
Assert.That(pixelForeground.Weight, Is.EqualTo(FontWeight.Normal));
Assert.That(pixelForeground.Style, Is.EqualTo(FontStyle.Normal));
Assert.That(pixelForeground.TextDecoration, Is.Null);
Assert.IsNull(pixelForeground.Weight);
Assert.IsNull(pixelForeground.Style);
Assert.IsNull(pixelForeground.TextDecoration);
}

[Test]
Expand Down
42 changes: 39 additions & 3 deletions src/Tests/Consolonia.Core.Tests/PixelTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,9 +59,9 @@ public void ConstructorDrawingBoxSymbolAndColor()
new PixelBackground(Colors.Blue));
Assert.That(pixel.Foreground.Symbol.Text, Is.EqualTo("┼"));
Assert.That(pixel.Foreground.Color, Is.EqualTo(Colors.Red));
Assert.That(pixel.Foreground.Style, Is.EqualTo(FontStyle.Normal));
Assert.That(pixel.Foreground.Weight, Is.EqualTo(FontWeight.Normal));
Assert.That(pixel.Foreground.TextDecoration, Is.Null);
Assert.IsNull(pixel.Foreground.Style);
Assert.IsNull(pixel.Foreground.Weight);
Assert.IsNull(pixel.Foreground.TextDecoration);
Assert.That(pixel.Background.Color, Is.EqualTo(Colors.Blue));
Assert.That(pixel.Background.Mode, Is.EqualTo(PixelBackgroundMode.Colored));
}
Expand Down Expand Up @@ -142,6 +142,42 @@ public void BlendColoredBackground()
Assert.That(newPixel.Background.Color, Is.EqualTo(Colors.Blue));
}

[Test]
public void BlendShadedBackground()
{
var pixel = new Pixel(new PixelForeground(new SimpleSymbol("x"), Colors.Gray),
new PixelBackground(Colors.White));
var pixel2 = new Pixel(new PixelBackground(PixelBackgroundMode.Shaded));
Pixel newPixel = pixel.Blend(pixel2);
Assert.True(newPixel.Foreground.Symbol.Text == "x");
// foreground should be lighter than original
Assert.True(newPixel.Foreground.Color.R < pixel.Foreground.Color.R &&
newPixel.Foreground.Color.G < pixel.Foreground.Color.G &&
newPixel.Foreground.Color.B < pixel.Foreground.Color.B);
// background should be darker than original
Assert.True(newPixel.Background.Color.R < pixel.Background.Color.R &&
newPixel.Background.Color.G < pixel.Background.Color.G &&
newPixel.Background.Color.B < pixel.Background.Color.B);
}

[Test]
public void BlendShadedBackground2()
{
var pixel = new Pixel(new PixelForeground(new SimpleSymbol("x"), Colors.Gray),
new PixelBackground(Colors.Black));
var pixel2 = new Pixel(new PixelBackground(PixelBackgroundMode.Shaded));
Pixel newPixel = pixel.Blend(pixel2);
Assert.True(newPixel.Foreground.Symbol.Text == "x");
// foreground should be darker than original
Assert.True(newPixel.Foreground.Color.R < pixel.Foreground.Color.R &&
newPixel.Foreground.Color.G < pixel.Foreground.Color.G &&
newPixel.Foreground.Color.B < pixel.Foreground.Color.B);
// background should be darker than original
Assert.True(newPixel.Background.Color.R > pixel.Background.Color.R &&
newPixel.Background.Color.G > pixel.Background.Color.G &&
newPixel.Background.Color.B > pixel.Background.Color.B);
}
tomlm marked this conversation as resolved.
Show resolved Hide resolved

[Test]
public void HashCode()
{
Expand Down
Loading