Skip to content

Conversation

@EbiseLutica
Copy link
Collaborator

@EbiseLutica EbiseLutica commented Jul 12, 2025

fix #52

  • Texture2Dに UvStart UvEnd` プロパティを追加
  • OpenGLのテクスチャレンダラーが、 UvStart UvEnd プロパティを用いるよう変更
  • OpenGLのテクスチャファクトリの LoadSpriteSheet が、本機能を用いて1つだけハンドルを生成し、複数のTexture2Dで使い回すように

@EbiseLutica
Copy link
Collaborator Author

NineSliceSpriteについても同様の処理を行うか?

@EbiseLutica EbiseLutica requested a review from Copilot July 13, 2025 02:43
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Adds UV coordinate support to Texture2D, updates the OpenGL pipeline to use these UVs, and optimizes sprite sheet loading by reusing a single GPU texture handle.

  • Introduce UvStart/UvEnd properties and overload constructors in Texture2D
  • Update OpenGLTextureFactory.LoadSpriteSheet to generate one GL handle and slice UVs per sub-texture
  • Modify GL renderer helper to stream vertex+UV data into a dynamic VBO per draw

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
Promete/Graphics/Texture2D.cs Added UV coordinate properties and new constructors
Promete/Windowing/GLDesktop/OpenGLTextureFactory.cs New sprite sheet loader uses UV slicing and single GL handle
Promete/Nodes/Renderer/GL/Helper/GLTextureRendererHelper.cs Switched VBO to dynamic draw and inject per-sprite UVs
Promete/Promete.csproj Bumped package version
Promete/Graphics/FrameBuffer.cs Removed unused using directives
Promete.Example/examples/graphics/sprite_sheet.cs New demo showing sprite sheet loading and usage
Comments suppressed due to low confidence (4)

Promete.Example/examples/graphics/sprite_sheet.cs:9

  • [nitpick] Class names should follow PascalCase. Rename sprite_sheet to SpriteSheetDemo or similar for consistency.
public class sprite_sheet : Scene

Promete/Graphics/Texture2D.cs:24

  • [nitpick] Per .NET naming conventions, acronyms longer than one letter should be uppercase. Rename UvStart/UvEnd to UVStart/UVEnd.
    public Vector UvStart { get; }

Promete/Windowing/GLDesktop/OpenGLTextureFactory.cs:80

  • The new sprite sheet loader logic isn’t covered by any existing tests. Consider adding unit or integration tests to verify correct UV slicing and exception cases.
    private Texture2D[] LoadSpriteSheet(Image bmp, int horizontalCount, int verticalCount, VectorInt size)

Promete/Promete.csproj:13

  • [nitpick] Verify that the new pre-release suffix (-exp.2) aligns with your project’s versioning strategy and CI/CD processes.
        <Version>1.0.1-exp.2</Version>

{
var width = (float)bmp.Width;
var height = (float)bmp.Height;
var handle = LoadFromImageSharpImage(bmp).Handle;
Copy link

Copilot AI Jul 13, 2025

Choose a reason for hiding this comment

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

The call to LoadFromImageSharpImage(bmp) allocates a Texture2D that never gets disposed, leaking a GPU handle. Capture the returned Texture2D, retrieve its Handle, then call Dispose() on it explicitly.

Suggested change
var handle = LoadFromImageSharpImage(bmp).Handle;
var texture = LoadFromImageSharpImage(bmp);
var handle = texture.Handle;
texture.Dispose();

Copilot uses AI. Check for mistakes.
Comment on lines +94 to +95
if (px + size.X > width) throw new ArgumentException(null, nameof(horizontalCount));
if (py + size.Y > height) throw new ArgumentException(null, nameof(verticalCount));
Copy link

Copilot AI Jul 13, 2025

Choose a reason for hiding this comment

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

Using ArgumentException with a null message and parameter name "horizontalCount" is confusing here. Prefer ArgumentOutOfRangeException with a clear message or use the correct parameter name (e.g., size or computed index).

Suggested change
if (px + size.X > width) throw new ArgumentException(null, nameof(horizontalCount));
if (py + size.Y > height) throw new ArgumentException(null, nameof(verticalCount));
if (px + size.X > width) throw new ArgumentOutOfRangeException(nameof(size), $"Computed X index ({px + size.X}) exceeds image width ({width}).");
if (py + size.Y > height) throw new ArgumentOutOfRangeException(nameof(size), $"Computed Y index ({py + size.Y}) exceeds image height ({height}).");

Copilot uses AI. Check for mistakes.
Comment on lines +82 to +105
var width = (float)bmp.Width;
var height = (float)bmp.Height;
var handle = LoadFromImageSharpImage(bmp).Handle;

var textures = new Texture2D[verticalCount * horizontalCount];
for (var y = 0; y < verticalCount; y++)
{
for (var x = 0; x < horizontalCount; x++)
{
var px = x * size.X;
var py = y * size.Y;

if (px + size.X > width) throw new ArgumentException(null, nameof(horizontalCount));
if (py + size.Y > height) throw new ArgumentException(null, nameof(verticalCount));

var uvStart = new Vector(px / width, py / height);
var uvEnd = new Vector((px + size.X) / width, (py + size.Y) / height);

textures[y * horizontalCount + x] = new Texture2D(handle, size, DisposeTexture, uvStart, uvEnd);
}
}

bmp.Dispose();
return textures;
Copy link

Copilot AI Jul 13, 2025

Choose a reason for hiding this comment

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

Manual Dispose at the end won’t run if an exception is thrown earlier. Wrap the Image in a using or try/finally to guarantee disposal on error.

Suggested change
var width = (float)bmp.Width;
var height = (float)bmp.Height;
var handle = LoadFromImageSharpImage(bmp).Handle;
var textures = new Texture2D[verticalCount * horizontalCount];
for (var y = 0; y < verticalCount; y++)
{
for (var x = 0; x < horizontalCount; x++)
{
var px = x * size.X;
var py = y * size.Y;
if (px + size.X > width) throw new ArgumentException(null, nameof(horizontalCount));
if (py + size.Y > height) throw new ArgumentException(null, nameof(verticalCount));
var uvStart = new Vector(px / width, py / height);
var uvEnd = new Vector((px + size.X) / width, (py + size.Y) / height);
textures[y * horizontalCount + x] = new Texture2D(handle, size, DisposeTexture, uvStart, uvEnd);
}
}
bmp.Dispose();
return textures;
using (bmp)
{
var width = (float)bmp.Width;
var height = (float)bmp.Height;
var handle = LoadFromImageSharpImage(bmp).Handle;
var textures = new Texture2D[verticalCount * horizontalCount];
for (var y = 0; y < verticalCount; y++)
{
for (var x = 0; x < horizontalCount; x++)
{
var px = x * size.X;
var py = y * size.Y;
if (px + size.X > width) throw new ArgumentException(null, nameof(horizontalCount));
if (py + size.Y > height) throw new ArgumentException(null, nameof(verticalCount));
var uvStart = new Vector(px / width, py / height);
var uvEnd = new Vector((px + size.X) / width, (py + size.Y) / height);
textures[y * horizontalCount + x] = new Texture2D(handle, size, DisposeTexture, uvStart, uvEnd);
}
}
return textures;
}

Copilot uses AI. Check for mistakes.
Comment on lines +113 to +115
// VBOへ頂点データを書き込み
gl.BindBuffer(BufferTargetARB.ArrayBuffer, _vbo);
gl.BufferSubData<float>(BufferTargetARB.ArrayBuffer, 0, vertices);
Copy link

Copilot AI Jul 13, 2025

Choose a reason for hiding this comment

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

[nitpick] Uploading vertex data per sprite draw can become a bottleneck when rendering many sprites. Consider batching multiple sprites or using instanced rendering to reduce buffer updates.

Suggested change
// VBOへ頂点データを書き込み
gl.BindBuffer(BufferTargetARB.ArrayBuffer, _vbo);
gl.BufferSubData<float>(BufferTargetARB.ArrayBuffer, 0, vertices);
// バッチ用の頂点データを蓄積
_batchedVertices.AddRange(vertices.ToArray());
// バッチが一定サイズに達した場合、VBOへ頂点データを書き込み
if (_batchedVertices.Count >= _batchSize)
{
gl.BindBuffer(BufferTargetARB.ArrayBuffer, _vbo);
gl.BufferSubData<float>(BufferTargetARB.ArrayBuffer, 0, _batchedVertices.ToArray());
_batchedVertices.Clear();
}

Copilot uses AI. Check for mistakes.

public override void OnDestroy()
{
_textures[0].Dispose();
Copy link

Copilot AI Jul 13, 2025

Choose a reason for hiding this comment

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

Only the first sub-texture is disposed, leaking the rest. Either call Dispose on all textures or implement a proper cleanup for the shared handle.

Suggested change
_textures[0].Dispose();
foreach (var texture in _textures)
{
texture.Dispose();
}

Copilot uses AI. Check for mistakes.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

_texturesのすべてのテクスチャのハンドルIDが共通しているので、最初のものだけを削除する対応でokなはず

@EbiseLutica
Copy link
Collaborator Author

macOS上で動かしたときに、タイルマップの処理が異常に遅くなってしまうようになった

実装に問題がありそうなので一旦マージは見送る(Draft扱いとする)

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Texture2DにUV座標をもたせる

2 participants