Skip to content

[feat] Add WriteTexture to Queue#95

Merged
kolkov merged 3 commits intogogpu:mainfrom
Carmen-Shannon:feat/wire-write-texture
Mar 7, 2026
Merged

[feat] Add WriteTexture to Queue#95
kolkov merged 3 commits intogogpu:mainfrom
Carmen-Shannon:feat/wire-write-texture

Conversation

@Carmen-Shannon
Copy link
Contributor

No description provided.

@Carmen-Shannon Carmen-Shannon requested a review from kolkov as a code owner March 6, 2026 22:24
@Carmen-Shannon
Copy link
Contributor Author

@kolkov Hi, first off thanks for your work on this library, I'm working on migrating off of a cgo webgpu implementation on to your backend and I noticed there was no public API for the WriteTexture method I'm using in my project.

I had a couple of questions;
1.) I added the type alias for TextureDataLayout with a local helper to convert it to the correct hal type before passing it to the hal backend but wanted to confirm placement, does it belong in the types.go file with the other aliases?
2.) Same kind of question but I figured the ImageCopyTexture type would be better implemented as a direct struct in the texture.go file to support the existing Texture API. Would it be better placed in types.go? I feel like the answer is no because of the toHAL method implemented on it.

@kolkov
Copy link
Contributor

kolkov commented Mar 7, 2026

Hey @Carmen-Shannon, thanks for the PR and for choosing to migrate to our Pure Go backend! Really glad to see someone building real projects on top of wgpu — a 3D engine is exactly the kind of use case we want to support well.

The WriteTexture gap in the public API was a real oversight on our side, so thanks for catching it and putting together the implementation.

I've reviewed the code and it's solid overall. A few suggestions to align with our existing patterns — happy to discuss any of these:

1. ImageCopyTexture placement

All structs with toHAL() converters currently live in descriptor.go — that's where BufferDescriptor, TextureDescriptor, SamplerDescriptor, RenderPassDescriptor, etc. are (12 types total). Moving ImageCopyTexture and its toHAL() there would keep things consistent. The struct itself is well-designed — own type with *Texture reference and HAL conversion is exactly the right approach.

2. Origin3D direct assignment

Since Origin3D is a type alias (type Origin3D = hal.Origin3D in descriptor.go), the field-by-field copy in toHAL() can be simplified to just:

Origin: i.Origin,

Same type on both sides, direct assignment works.

3. TextureDataLayout → ImageDataLayout

Looking at how we handle similar types — Extent3D and Origin3D are aliased directly from hal in descriptor.go:

type Extent3D = hal.Extent3D
type Origin3D = hal.Origin3D

The same approach works for the layout type:

type ImageDataLayout = hal.ImageDataLayout

This eliminates the need for textureDataLayoutToHAL() entirely — you can pass layout straight through to q.hal.WriteTexture(). The TextureDataLayout alias from gputypes in types.go could be dropped in favor of this.

4. Validation style (minor)

The existing WriteBuffer and ReadBuffer in the same file use a more compact validation style:

if q.hal == nil || buffer == nil {
    return fmt.Errorf("wgpu: WriteBuffer: queue or buffer is nil")
}

The 5 separate checks work fine functionally, but matching the compact style would keep the file consistent. The HAL backends already validate their own invariants (nil texture, empty data, etc.), so the public layer mainly needs to guard against nil queue and nil destination.


Let me know what you think. These are all straightforward changes and the core logic is good. Looking forward to getting this merged!

@Carmen-Shannon
Copy link
Contributor Author

I'll make those changes now to get this aligned better with the existing codebase, thanks for your feedback!

@Carmen-Shannon
Copy link
Contributor Author

Carmen-Shannon commented Mar 7, 2026

@kolkov In regards to the validation style;

I noticed that ReadBuffer isn't using the compacted syntax, I'll update that in this PR as well since it's right there to align it with WriteBuffer.

I would just like a little clarification as to what should be checked in WriteTexture defensively. Would you prefer to have the 5 checks all merged into a single check with one fmt.Errorf return? IMO this would create a bit of ambiguity from the caller's perspective when debugging, since they would have to figure out which of their provided arguments is nil or invalid, rather than leaving the 5 checks with specific errors for each check. I could also combine certain checks where appropriate but I don't see a distinct relationship where combining them makes a ton of sense here.

quick edit: combining the q.hal and pointer arguments makes sense logically to me

@kolkov
Copy link
Contributor

kolkov commented Mar 7, 2026

By the way, feel free to force push to keep the commit history clean — we squash merge anyway, so a single clean commit is perfectly fine. Totally up to you though.

@kolkov
Copy link
Contributor

kolkov commented Mar 7, 2026

Good point — you're right that specific error messages are better for debugging. The compact style in WriteBuffer is arguably the weaker pattern, not the one to follow.

Your approach of combining q.hal + dst into one check while keeping the rest granular makes sense. That way callers get clear feedback on what's wrong without digging through the code.

One ask: please don't change ReadBuffer in this PR — let's keep the scope focused on WriteTexture. We can clean up the existing methods in a separate PR later.

Thanks for pushing back on this, it's the right call.

@kolkov
Copy link
Contributor

kolkov commented Mar 7, 2026

Your questions actually prompted some bigger thinking on our side. The public API layer was the last thing we built and we cut some corners — your PR exposed real inconsistencies in how we handle validation, error messages, and type design.

We've opened two research issues to figure out the right approach going forward:

If you have thoughts on any of this — especially from the perspective of someone migrating to our library — we'd really value your input on those issues.

Copy link
Contributor

@kolkov kolkov left a comment

Choose a reason for hiding this comment

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

Looks great, thanks for the quick turnaround! All the key changes are in place — ImageCopyTexture in descriptor.go, ImageDataLayout alias, direct Origin3D assignment, and nice tests too.

One minor nit: TextureDataLayout in types.go is unused now that WriteTexture uses ImageDataLayout. Feel free to drop it, or we can clean it up later — not blocking.

Approving, good to merge once CI passes.

@Carmen-Shannon
Copy link
Contributor Author

I will take a look at both of those threads and see if I can contribute any insight as I get further along with my own re-factor to get on gogpu 100%. I'm sure I'll run into something that'll prompt some good feedback for you.

I fixed the descriptor.go file as I forgot to fmt it.

@codecov
Copy link

codecov bot commented Mar 7, 2026

Codecov Report

❌ Patch coverage is 47.36842% with 10 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
queue.go 27.27% 4 Missing and 4 partials ⚠️
descriptor.go 75.00% 1 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

@kolkov kolkov merged commit 0761e19 into gogpu:main Mar 7, 2026
11 checks passed
kolkov added a commit that referenced this pull request Mar 7, 2026
- naga v0.14.6: MSL pass-through globals fix (naga#40)
- CHANGELOG: WriteTexture (#95 by @Carmen-Shannon) + naga update
kolkov added a commit that referenced this pull request Mar 7, 2026
- naga v0.14.6: MSL pass-through globals fix (naga#40)
- CHANGELOG: WriteTexture (#95 by @Carmen-Shannon) + naga update
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.

2 participants