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.

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

Texture wrap mode ignored when atlas already bound #6409

Closed
Jumprocks1 opened this issue Nov 5, 2024 · 1 comment · Fixed by #6410
Closed

Texture wrap mode ignored when atlas already bound #6409

Jumprocks1 opened this issue Nov 5, 2024 · 1 comment · Fixed by #6410

Comments

@Jumprocks1
Copy link
Contributor

Found on 2024.523.0, confirmed and tested on 2024.1025.0

When a texture is bound through Renderer.BindTexture, the wrap mode is ignored if the texture is a part of the currently bound atlas.

The logic for this is in:

public bool BindTexture(Texture texture, int unit, WrapMode? wrapModeS, WrapMode? wrapModeT)
{
ObjectDisposedException.ThrowIf(!texture.Available, texture);
if (texture is TextureWhitePixel && lastBoundTextureIsAtlas[unit])
{
setWrapMode(wrapModeS ?? texture.WrapModeS, wrapModeT ?? texture.WrapModeT);
// We can use the special white space from any atlas texture.
return true;
}
texture.NativeTexture.Upload();
bool didBind = BindTexture(texture.NativeTexture, unit, wrapModeS ?? texture.WrapModeS, wrapModeT ?? texture.WrapModeT);
lastBoundTextureIsAtlas[unit] = texture.IsAtlasTexture;
return didBind;
}
/// <summary>
/// Binds a native texture. Generally used by internal components of renderer implementations.
/// </summary>
/// <param name="texture">The native texture to bind.</param>
/// <param name="unit">The sampling unit in which the texture is to be bound.</param>
/// <param name="wrapModeS">The texture's horizontal wrap mode.</param>
/// <param name="wrapModeT">The texture's vertex wrap mode.</param>
/// <returns>Whether the texture was successfully bound.</returns>
public bool BindTexture(INativeTexture texture, int unit = 0, WrapMode wrapModeS = WrapMode.None, WrapMode wrapModeT = WrapMode.None)
{
if (lastActiveTextureUnit == unit && lastBoundTexture[unit] == texture)
return true;
FlushCurrentBatch(FlushBatchSource.BindTexture);
if (!SetTextureImplementation(texture, unit))
return false;
setWrapMode(wrapModeS, wrapModeT);
lastBoundTexture[unit] = texture;
lastBoundTextureIsAtlas[unit] = false;
lastActiveTextureUnit = unit;
FrameStatistics.Increment(StatisticsCounterType.TextureBinds);
texture.TotalBindCount++;
return true;
}

Notice that if the INativeTexture is already bound, it skips setting the wrap mode. This is incorrect since a single INativeTexture (ie. an atlas) can have textures with different wrap modes in it.

There was already a special case added for TextureWhitePixel in #6350 but that does not fix the issue for other textures.

Here's a test case to reproduce the issue. Note that I had to downscale "sample-texture.png" from 512x512 to 256x256 since the 512x512 size does not allow fitting 2 of them in the 1024x1024 atlas.

// Copyright (c) ppy Pty Ltd <[email protected]>. Licensed under the MIT Licence.
// See the LICENCE file in the repository root for full licence text.

using osu.Framework.Allocation;
using osu.Framework.Graphics.Primitives;
using osu.Framework.Graphics.Sprites;
using osu.Framework.Graphics.Textures;

namespace osu.Framework.Tests.Visual
{
    public partial class TestSceneSpriteWrap : FrameworkTestScene
    {
        [BackgroundDependencyLoader]
        private void load(TextureStore store)
        {
            float y = 0f;
            Sprite makeSprite(WrapMode wrapModeS)
            {
                var o = new Sprite
                {
                    Width = 100,
                    Height = 50,
                    Texture = store.Get("sample-texture-256", wrapModeS, WrapMode.None),
                    TextureRectangle = new RectangleF(0f, 0f, 0.5f, 1f),
                    Y = y
                };
                y += 50;
                return o;
            }
            // sets Renderer.CurrentWrapMode to ClampToBorder and binds the atlas for sample-texture-256
            Add(makeSprite(WrapMode.ClampToBorder));
            // this wrap mode will be ignored and drawn as ClampToBorder instead
            Add(makeSprite(WrapMode.Repeat));
            Add(new SpriteText { Y = y, Text = "Text to cause bind to different texture atlas" });
            y += 20;
            // We should expect this to look identical to the second sprite, but it ends up
            // looking different due to the texture bind order.
            // This sprite is drawn correctly.
            Add(makeSprite(WrapMode.Repeat));
        }
    }
}

image

This is my recommended fix, though I don't know much about the intricacies of the render code:

diff --git a/osu.Framework/Graphics/Rendering/Renderer.cs b/osu.Framework/Graphics/Rendering/Renderer.cs
index 8066d379e..01a8a1f39 100644
--- a/osu.Framework/Graphics/Rendering/Renderer.cs
+++ b/osu.Framework/Graphics/Rendering/Renderer.cs
@@ -818,7 +818,10 @@ public bool BindTexture(Texture texture, int unit, WrapMode? wrapModeS, WrapMode
         public bool BindTexture(INativeTexture texture, int unit = 0, WrapMode wrapModeS = WrapMode.None, WrapMode wrapModeT = WrapMode.None)
         {
             if (lastActiveTextureUnit == unit && lastBoundTexture[unit] == texture)
+            {
+                setWrapMode(wrapModeS, wrapModeT);
                 return true;
+            }
 
             FlushCurrentBatch(FlushBatchSource.BindTexture);
 
@smoogipoo
Copy link
Contributor

Ouch. Your fix looks fine, feel free to push it + a test case alongside the one added in #6350.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants