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

Revisit "gpu resource" naming #24

Closed
cart opened this issue Jun 11, 2020 · 2 comments
Closed

Revisit "gpu resource" naming #24

cart opened this issue Jun 11, 2020 · 2 comments

Comments

@cart
Copy link
Member

cart commented Jun 11, 2020

Right now the naming used for "gpu resource" concepts is confusing and sometimes conflicting:

  • RenderResourceId
    • a backend-agnostic handle to a gpu resource
  • RenderResourceSet
    • a set of RenderResourceIds uniquely identifiable via a hash of the contained RenderResourceIds. these eventually get turned into a wgpu::BindGroup by the wgpu RenderResourceContext
  • RenderResourceAssignments
    • A mapping from "shader binding" string names to RenderResourceIds. It also contains named vertex buffer / index buffer assignments. Used alongside pipeline layouts to generate RenderResourceSets. The RenderResourceSets are cached and are only re-generated when a RenderResourceAssignment is changed.
  • render_resource::RenderResource
    • a trait that allows extracting shader resource/uniform information from a rust data type, which can then be used by a render backend to create a render resource (indexed by a RenderResourceId)
  • render_resource::RenderResources
    • a derivable trait that allows access to a set of render_resource::RenderResource trait impls indexed by their string names
  • render_resource_context::RenderResourceContext
    • a trait implemented by a render backend that provides thread safe read/write operations on gpu resources indexed by RenderResourceIds.
  • render_resource_context::RenderResources
    • a legion resource that provides a downcastable boxed RenderResourceContext
    • Note: this name conflicts with render_resource::RenderResources

Some thoughts:

  • First and foremost, there shouldn't be name conflicts. This will be confusing for people
  • RenderResourceSet conceptually maps to BindGroup. It probably makes sense to rename it.
  • "Render Resource" is a made up concept, and maybe not a useful one. Maybe instead of having a single RenderResourceId type, it could be broken up into BufferId, TextureId, SamplerId, etc. This gives additional type safety by default. When they need to be conceptually grouped (ex: bindings), a wrapper type can be used.
  • "Uniform" is a more familiar term than "Render Resource". I didnt want to call these uniforms become not all "Render Resources" are actually Uniforms. But maybe thats ok. Its worth considering a rename from RenderResourceAssignments to UniformAssignments or just Uniforms. If that happens it probably makes sense to move vertex buffer assignments to a different type. The same argument applies to render_resource:RenderResource and render_resource::RenderResources.
  • The render_resource_context::RenderResourceContext / render_resource_context::RenderResources distinction is confusing. RenderResources is just a wrapper over the Box-ed RenderResourceContext. Maybe its better to just insert the Box<dyn RenderResourceContext> directly into legion's resource collection. Accessing resources the resource by its type wouldnt be as clean, but it would remove another type that people need to worry about.

In general the goal here should be to improve clarity and remove as much "invented jargon" as possible. Types should be self-describing.

@cart
Copy link
Member Author

cart commented Jun 14, 2020

I just made the following changes:

  • Split out RenderResourceId into BufferId, TextureId, and SamplerId and used them wherever possible. RenderResourceId is now an enum of BufferId, TextureId, and SamplerId and is still used in a few places.
  • RenderResourceAssignments -> RenderResourceBindings
  • RenderResourceSet -> BindGroup

The RenderResources container type still needs a new name.

@cart
Copy link
Member Author

cart commented Jun 14, 2020

Removed the RenderResources container type. I think we're in a much better spot now, so I'm closing this

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

No branches or pull requests

1 participant