Skip to content

fix Duplicate DrawString #604

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

Open
7aGiven wants to merge 4 commits into
base: master
Choose a base branch
from
Open

fix Duplicate DrawString #604

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b602f0c
fix Duplicate DrawString
7aGiven Jul 18, 2026
0606cd7
Apply suggestions from code review
7aGiven Jul 18, 2026
218fe0a
change interface to generic to avoid boxing
7aGiven Jul 19, 2026
fb9eaa1
Don't use ref for small struct
7aGiven Jul 20, 2026
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
178 changes: 27 additions & 151 deletions src/Graphics/SpriteBatch.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -708,149 +708,12 @@ public void DrawString(
SpriteEffects effects,
float layerDepth
) {
/* FIXME: This method is a duplicate of DrawString(string)!
* The only difference is how we iterate through the StringBuilder.
* We don't use ToString() since it generates garbage.
* -flibit
*/
CheckBegin("DrawString");
if (text == null)
{
throw new ArgumentNullException("text");
}
if (text.Length == 0)
{
return;
}
effects &= (SpriteEffects) 0x03;

/* We pull all these internal variables in at once so
* anyone who wants to use this file to make their own
* SpriteBatch can easily replace these with reflection.
* -flibit
*/
Texture2D textureValue = spriteFont.textureValue;
List<Rectangle> glyphData = spriteFont.glyphData;
List<Rectangle> croppingData = spriteFont.croppingData;
List<Vector3> kerning = spriteFont.kerning;
Dictionary<char, int> characterIndexMap = spriteFont.characterIndexMap;

// FIXME: This needs an accuracy check! -flibit

// Calculate offsets/axes, using the string size for flipped text
Vector2 baseOffset = origin;
float axisDirX = axisDirectionX[(int) effects];
float axisDirY = axisDirectionY[(int) effects];
float axisDirMirrorX = 0.0f;
float axisDirMirrorY = 0.0f;
if (effects != SpriteEffects.None)
{
Vector2 size = spriteFont.MeasureString(text);
baseOffset.X -= size.X * axisIsMirroredX[(int) effects];
baseOffset.Y -= size.Y * axisIsMirroredY[(int) effects];
axisDirMirrorX = axisIsMirroredX[(int) effects];
axisDirMirrorY = axisIsMirroredY[(int) effects];
}

Vector2 curOffset = Vector2.Zero;
bool firstInLine = true;
for (int i = 0; i < text.Length; i += 1)
{
char c = text[i];

// Special characters
if (c == '\r')
{
continue;
}
if (c == '\n')
{
curOffset.X = 0.0f;
curOffset.Y += spriteFont.LineSpacing;
firstInLine = true;
continue;
}

/* Get the List index from the character map, defaulting to the
* DefaultCharacter if it's set.
*/
int index;
if (!characterIndexMap.TryGetValue(c, out index))
{
if (!spriteFont.DefaultCharacter.HasValue)
{
throw new ArgumentException(
"Text contains characters that cannot be" +
" resolved by this SpriteFont.",
"text"
);
}
index = characterIndexMap[spriteFont.DefaultCharacter.Value];
}

/* For the first character in a line, always push the width
* rightward, even if the kerning pushes the character to the
* left.
*/
Vector3 cKern = kerning[index];
if (firstInLine)
{
curOffset.X += Math.Abs(cKern.X);
firstInLine = false;
}
else
{
curOffset.X += spriteFont.Spacing + cKern.X;
}

// Calculate the character origin
Rectangle cCrop = croppingData[index];
Rectangle cGlyph = glyphData[index];
float offsetX = baseOffset.X + (
curOffset.X + cCrop.X
) * axisDirX;
float offsetY = baseOffset.Y + (
curOffset.Y + cCrop.Y
) * axisDirY;
if (effects != SpriteEffects.None)
{
offsetX += cGlyph.Width * axisDirMirrorX;
offsetY += cGlyph.Height * axisDirMirrorY;
}

// Draw!
float sourceW = Math.Sign(cGlyph.Width) * Math.Max(
Math.Abs(cGlyph.Width),
MathHelper.MachineEpsilonFloat
) / (float) textureValue.Width;
float sourceH = Math.Sign(cGlyph.Height) * Math.Max(
Math.Abs(cGlyph.Height),
MathHelper.MachineEpsilonFloat
) / (float) textureValue.Height;
PushSprite(
textureValue,
cGlyph.X / (float) textureValue.Width,
cGlyph.Y / (float) textureValue.Height,
sourceW,
sourceH,
position.X,
position.Y,
cGlyph.Width * scale.X,
cGlyph.Height * scale.Y,
color,
offsetX / sourceW / (float) textureValue.Width,
offsetY / sourceH / (float) textureValue.Height,
(float) Math.Sin(rotation),
(float) Math.Cos(rotation),
layerDepth,
(int) effects
);

/* Add the character width and right-side
* bearing to the line width.
*/
curOffset.X += cKern.Y + cKern.Z;
}
SpriteFont.IReadOnlyCharList view = new SpriteFont.StringBuilderView(text);
DrawString(spriteFont, ref view, position, color, rotation, origin, scale, effects, layerDepth);

@kg kg Jul 18, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

As mentioned in another comment, by construction this boxes the StringBuilderView to store it in a reference-typed (IReadOnlyCharList) local, then passes a reference to the reference, which is unnecessary. Either make DrawString generic so you can pass structs into it by-ref or drop the ref here.

@7aGiven 7aGiven Jul 19, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thank you, I follow your suggestion that use generic method instead of interface argument. So no boxing now.

}

public void DrawString(
Expand Down Expand Up @@ -907,15 +770,31 @@ public void DrawString(
SpriteEffects effects,
float layerDepth
) {
/* FIXME: This method is a duplicate of DrawString(StringBuilder)!
* The only difference is how we iterate through the string.
* -flibit
*/
CheckBegin("DrawString");
if (text == null)
{
throw new ArgumentNullException("text");
}
SpriteFont.IReadOnlyCharList view = new SpriteFont.StringView(text);
DrawString(spriteFont, ref view, position, color, rotation, origin, scale, effects, layerDepth);
}

#endregion

#region Private Methods

private void DrawString(
SpriteFont spriteFont,
ref SpriteFont.IReadOnlyCharList text,
Vector2 position,
Color color,
float rotation,
Vector2 origin,
Vector2 scale,
SpriteEffects effects,
float layerDepth
)
{
CheckBegin("DrawString");
if (text.Length == 0)
{
return;
Expand Down Expand Up @@ -943,7 +822,7 @@ float layerDepth
float axisDirMirrorY = 0.0f;
if (effects != SpriteEffects.None)
{
Vector2 size = spriteFont.MeasureString(text);
Vector2 size = spriteFont.MeasureString(ref text);
baseOffset.X -= size.X * axisIsMirroredX[(int) effects];
baseOffset.Y -= size.Y * axisIsMirroredY[(int) effects];
axisDirMirrorX = axisIsMirroredX[(int) effects];
Expand All @@ -952,8 +831,10 @@ float layerDepth

Vector2 curOffset = Vector2.Zero;
bool firstInLine = true;
foreach (char c in text)
for (int i = 0; i < text.Length; i += 1)
{
char c = text[i];

// Special characters
if (c == '\r')
{
Expand Down Expand Up @@ -1048,11 +929,6 @@ float layerDepth
curOffset.X += cKern.Y + cKern.Z;
}
}

#endregion

#region Private Methods

private unsafe void PushSprite(
Texture2D texture,
float sourceX,
Expand Down
Loading
Loading