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

Logic in materials is based on details about the wgpu renderer #272

Closed
almarklein opened this issue Mar 28, 2022 · 2 comments · Fixed by #762
Closed

Logic in materials is based on details about the wgpu renderer #272

almarklein opened this issue Mar 28, 2022 · 2 comments · Fixed by #762
Labels
discussion Something to discuss

Comments

@almarklein
Copy link
Member

almarklein commented Mar 28, 2022

While looking at #257 I ran into some abstraction leaks. We try to make the worldObject/geometry/material logic free from logic about the renderer. Even though we only have one real renderer (wgpu) this abstraction has helped us (I think) keep the code clean. But there are some things where it's off:

Last updated 18-04-2023

  • Some material properties are stored just as a property, while others are stored in the uniform buffer. Ideally this is a detail of the renderer.
  • Same for world object, though so far only the render_mask property.
  • Buffer and Texture set a few wgpu-specific private attributes. Theses are only used by the renderer, though a user could technically use _wgpu_usage when needed.
  • Actually, there are valid cases where a user should be able to set wgpu_usage, see API for specifying buffer and texture wgpu usage #574.
  • Buffer and Texture accept a format that is generic, but also accept a wgpu.BufferFormat or wgpu.TextureFormat respectively.
  • The resource registry for buffers and textures uses wgpu-specific attributes of resources (writing this as I am working on More efficient Buffer and Texture updates #508, so maybe that has changed).
  • Material._wgpu_get_pick_info()
  • WorldObject._wgpu_get_pick_info()
  • utils/cube_camera.py -> uses/wraps a WgpuRenderer.
  • The show() util uses the WgpuRenderer, but fair enough since that's the default.

I'm not sure what best to do about this. Just accept it, we use the abstraction as a "rule of thumb" to keep on track? Or fix it, by moving the offending logic to the renderer?

@Korijn
Copy link
Collaborator

Korijn commented Mar 29, 2022

You're right, it's all supposed to be managed and tracked as part of the renderer. But we don't have to fix this right away.

@Korijn
Copy link
Collaborator

Korijn commented Mar 6, 2024

Decisions:

  • wgpu calls will be confined to renderers/backends.
  • render state may be managed on public API objects.
  • wgpu enums/flags may be used in public API objects. (e.g. culling mode)
  • We will embrace the fact that pygfx is based on the class of rendering backends that uses WGPU and similar implementations. As such "leaking abstractions" like properties that go into buffers or trigger shader compilations are OK to have in public API objects. It will be something that all foreseeable rendering backends require anyway.
  • As an example, the SVG renderer can still be implemented even if wgpu flags are used in public API objects, since wgpu can be imported without importing the rust binaries and without creating GPU devices/objects. We will not directly further invest in the SVG but it is free for community members to further develop it.

Actions:

  • Review the codebase to identify violations of these architectural constraints, and correct them
  • Review the codebase and address opportunities to simplify abstractions, where we are attempting to abstract away wgpu internal details, where it is actually only adding maintenance overhead and obfuscating underlying concepts

@almarklein almarklein mentioned this issue Apr 10, 2024
4 tasks
@almarklein almarklein mentioned this issue May 21, 2024
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion Something to discuss
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants