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

Make unstruct resolvable and introduce root.unwrap(TgpuVertexLayout) #781

Open
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

reczkok
Copy link
Collaborator

@reczkok reczkok commented Jan 14, 2025

  • Tests for unstruct resolution
  • Tests for TgpuVertexLayout unwrapping
  • Update docs
  • Check if feasible to type check if every attribute has a location

Copy link

codesandbox-ci bot commented Jan 14, 2025

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

@reczkok
Copy link
Collaborator Author

reczkok commented Jan 24, 2025

  • Check if feasible to type check if every attribute has a location

I experimented with this:

export type HasAllPropsWithLocations<T> = T extends TgpuVertexLayout<infer U>
  ? HasAllPropsWithLocations<U>
  : T extends WgslStruct | Unstruct
    ? {
        [K in keyof T['propTypes']]: HasAllPropsWithLocations<
          T['propTypes'][K]
        >;
      }[keyof T['propTypes']] extends false
      ? false
      : true
    : T extends { elementType: unknown }
      ? HasAllPropsWithLocations<T['elementType']>
      : HasCustomLocation<T> extends false
        ? false
        : true;

but keep running into Type instantiation is excessively deep and possibly infinite error

@reczkok reczkok marked this pull request as ready for review January 27, 2025 11:22
@reczkok reczkok requested review from iwoplaza and mhawryluk and removed request for iwoplaza January 27, 2025 11:23
apps/typegpu-docs/src/content/docs/fundamentals/roots.mdx Outdated Show resolved Hide resolved
trianglePos: { uniform: d.arrayOf(TriangleInfoStruct, triangleAmount) },
colorPalette: { uniform: d.vec3f },
});
const vertexLayout = tgpu['~unstable'].vertexLayout((n) =>
Copy link
Collaborator

Choose a reason for hiding this comment

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

what do we do about [~unstable] in published examples. I'd say we either

  1. make vertexLayout stable, docs public and release 0.3.3 right after merging this PR
  2. make boids experimental
  3. do nothing

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm kinda a fan of option 1

packages/typegpu/src/core/resolve/resolveData.ts Outdated Show resolved Hide resolved
packages/typegpu/src/core/vertexLayout/vertexLayout.ts Outdated Show resolved Hide resolved
packages/typegpu/src/core/vertexLayout/vertexLayout.ts Outdated Show resolved Hide resolved
reczkok and others added 2 commits January 29, 2025 12:49
Co-authored-by: Marcin Hawryluk <70582973+mhawryluk@users.noreply.github.com>
@reczkok reczkok requested a review from mhawryluk January 29, 2025 12:12
{/* | `root.unwrap(resource: TgpuTexture)` | Returns a `GPUTexture`. | */}
{/* | `root.unwrap(resource: TgpuReadonlyTexture \| TgpuWriteonlyTexture \| TgpuMutableTexture \| TgpuSampledTexture)` | Returns a `GPUTextureView`. | */}

:::note
To unwrap a `TgpuVertexLayout` make sure that all its attributes are marked with the appropriate location.
Copy link
Collaborator

Choose a reason for hiding this comment

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

A suggestion, this wording might suggest more that:

  • It's something the dev has to do manually,
  • This usually happens automatically, but it's a requirement when trying to unwrap
Suggested change
To unwrap a `TgpuVertexLayout` make sure that all its attributes are marked with the appropriate location.
To unwrap a `TgpuVertexLayout` make sure to explicitly mark each of its attributes with the appropriate location using `d.location(...)`.

@@ -176,6 +176,108 @@ describe('tgpu resolve', () => {
);
});

it('should resolve an unstruct to its corresponding struct', () => {
const vertexInfo = d.unstruct({
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's stick to PascalCase for structs.

Suggested change
const vertexInfo = d.unstruct({
const VertexInfo = d.unstruct({

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.

3 participants