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

Add partial documentation of WGPUSurface. #319

Merged
merged 11 commits into from
Aug 28, 2024
Merged

Conversation

Kangz
Copy link
Collaborator

@Kangz Kangz commented Aug 23, 2024

Done are:

  • wgpuInstanceCreateSurface
  • wgpuSurfaceGetCapabilities

And all the descriptors / enums referenced by them, as well as a more detailed article with pseudo-spec and examples.

Remaining are:

  • wgpuSurfaceConfigure
  • wgpuSurfaceUnconfigure
  • wgpuSurfaceGetCurrentTexture
  • wgpuSurfacePresent

Done are:

 - wgpuInstanceCreateSurface
 - wgpuSurfaceGetCapabilities

And all the descriptors / enums referenced by them, as well as a more
detailed article with pseudo-spec and examples.

Remaining are:

 - wgpuSurfaceConfigure
 - wgpuSurfaceUnconfigure
 - wgpuSurfaceGetCurrentTexture
 - wgpuSurfacePresent
@Kangz
Copy link
Collaborator Author

Kangz commented Aug 26, 2024

Can you add comments instead? I was going to push the rest of the doc now because I had time before your review. Sorry :X

@Kangz
Copy link
Collaborator Author

Kangz commented Aug 26, 2024

Or actually, let me rebase on top of your edits.

@Kangz
Copy link
Collaborator Author

Kangz commented Aug 26, 2024

PTAL, this should be the full API documentation, with some TODOs of things to resolve.

@kainino0x
Copy link
Collaborator

Oops sorry, didn't realize you had more changes incoming!

Copy link
Collaborator

@kainino0x kainino0x left a comment

Choose a reason for hiding this comment

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

LGTM but should probably have review from someone who knows it better than me

webgpu.yml Outdated Show resolved Hide resolved
webgpu.yml Outdated Show resolved Hide resolved
webgpu.yml Outdated Show resolved Hide resolved
webgpu.yml Outdated Show resolved Hide resolved
webgpu.yml Outdated Show resolved Hide resolved
doc/articles/Surfaces.md Show resolved Hide resolved
webgpu.yml Outdated Show resolved Hide resolved
doc/articles/Surfaces.md Outdated Show resolved Hide resolved

The behavior of `::wgpuSurfaceConfigure``(surface, config)` is:

- If any of these validation steps fails, TODO: what should happen on failure?
Copy link
Collaborator

Choose a reason for hiding this comment

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

Probably it should follow the JS API, which:

  • throws a TypeError (= WGPUStatus_something probably?) for invalid texture formats
  • generates a device error (on config.device) for other cases

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'll leave this open for now as I think it requires more discussions than what we can do in this review.

doc/articles/Surfaces.md Outdated Show resolved Hide resolved
doc/articles/Surfaces.md Outdated Show resolved Hide resolved
doc/articles/Surfaces.md Outdated Show resolved Hide resolved
doc/articles/Surfaces.md Show resolved Hide resolved
doc/articles/Surfaces.md Outdated Show resolved Hide resolved
webgpu.h Show resolved Hide resolved
webgpu.h Show resolved Hide resolved
@Kangz
Copy link
Collaborator Author

Kangz commented Aug 28, 2024

Merging now, @kainino0x please ask wgpu folks to review it and let's file issue for any changes that we might need.

@Kangz Kangz merged commit 9bc2892 into webgpu-native:main Aug 28, 2024
4 checks passed
@Kangz Kangz deleted the surface-docs branch August 28, 2024 11:39
@kainino0x kainino0x added the !discuss Needs discussion (at meeting or online) label Aug 28, 2024
@kainino0x kainino0x removed the !discuss Needs discussion (at meeting or online) label Sep 16, 2024
Copy link
Collaborator

@eliemichel eliemichel left a comment

Choose a reason for hiding this comment

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

Minor remarks/typos, sorry I couldn't review earlier!

doc/articles/Surfaces.md Show resolved Hide resolved
doc/articles/Surfaces.md Show resolved Hide resolved
doc/articles/Surfaces.md Show resolved Hide resolved
doc/articles/Surfaces.md Show resolved Hide resolved
doc/articles/Surfaces.md Show resolved Hide resolved
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.

4 participants