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

Consider renaming SwapChainSurface.suboptimal to optimal. #257

Closed
Kangz opened this issue Jan 3, 2024 · 2 comments · Fixed by #338
Closed

Consider renaming SwapChainSurface.suboptimal to optimal. #257

Kangz opened this issue Jan 3, 2024 · 2 comments · Fixed by #338
Labels
has resolution Issue is resolved, just needs to be done presentation Presenting images to surfaces like windows and canvases

Comments

@Kangz
Copy link
Collaborator

Kangz commented Jan 3, 2024

A slight nit is that negative member names often end up creating double negations in code that are slightly harder to parse. Renaming suboptimal to optimal would help.

@kainino0x kainino0x added !discuss Needs discussion (at meeting or online) presentation Presenting images to surfaces like windows and canvases has resolution Issue is resolved, just needs to be done needs docs Non-trivial API contract needs to be documented. Orthogonal to open vs closed; remove when doc'd. labels Jan 3, 2024
@kainino0x
Copy link
Collaborator

Meeting Jan 4:

  • Underlying APIs use “suboptimal”
  • CF: If the code is hard to read, make it an enum?
  • RM: Could add it to the existing status enum
  • KN: Does suboptimal only matter when the status is Success?
  • Yes
  • KN: A little annoying to check for both success and suboptimal statuses. Can we have a quicker way to see if you got a texture? Oh, we do - we return nullptr if you didn’t get one. Should document this.
  • Merge into the status, and document that you don’t have to check the status if you just want to know if you got a texture.

Something like:

typedef enum WGPUSurfaceGetCurrentTextureStatus {
    WGPUSurfaceGetCurrentTextureStatus_SuccessOptimal = 0x00000000,
    WGPUSurfaceGetCurrentTextureStatus_SuccessSuboptimal = 0x00000001,
    WGPUSurfaceGetCurrentTextureStatus_Timeout = 0x00000002,
    // ...
} WGPUSurfaceGetCurrentTextureStatus WGPU_ENUM_ATTRIBUTE;

typedef struct WGPUSurfaceTexture {
    WGPUTexture texture;
    WGPUSurfaceGetCurrentTextureStatus status;
} WGPUSurfaceTexture WGPU_STRUCTURE_ATTRIBUTE;
  • GetCurrentTexture return structure
    • Simplify?
      • WGPUTexture wgpuSurfaceGetCurrentTexture(WGPUSurface, WGPUGetCurrentTextureStatus* statusOutOptional);
      • optional status since you don’t necessarily need it if you just check for a null texture
      • RM: Don’t think status pointer should be optional
      • WGPUTexture wgpuSurfaceGetCurrentTexture(WGPUSurface, WGPUGetCurrentTextureStatus* statusOut);
      • or flip it
      • WGPUGetCurrentTextureStatus wgpuSurfaceGetCurrentTexture(WGPUSurface, WGPUTexture* textureOut);

Or WGPUSurfaceTexture wgpuSurfaceGetCurrentTexture(WGPUSurface);
I've filed #261 for more on this topic.

@kainino0x kainino0x removed the !discuss Needs discussion (at meeting or online) label Jan 5, 2024
@eliemichel
Copy link
Collaborator

👍 For merging suboptimal into the status enum and specifying that the basic success test is texture != nullptr!

@kainino0x kainino0x removed the needs docs Non-trivial API contract needs to be documented. Orthogonal to open vs closed; remove when doc'd. label Oct 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
has resolution Issue is resolved, just needs to be done presentation Presenting images to surfaces like windows and canvases
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants