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

Extracted webgl implementation of Vertex and Index buffer into separate modules #3982

Merged
merged 6 commits into from
Feb 7, 2022

Conversation

mvaligursky
Copy link
Contributor

@mvaligursky mvaligursky commented Feb 4, 2022

Related to: #3986

This is part of refactoring plan, where all webgl specific functionality will move to graphics/webgl, allowing the WebGPU implementation.

  • first step is to refactor IndexBuffer and VertexBuffer, and the shared buffer functionality has moved to WebglBuffer (which will also be used to implement UniformBuffer in the future)
  • interfaces are private and are subject to change

@mvaligursky mvaligursky self-assigned this Feb 4, 2022
@mvaligursky mvaligursky added area: graphics Graphics related issue enhancement labels Feb 4, 2022
@mvaligursky mvaligursky requested a review from a team February 4, 2022 09:18
@mvaligursky mvaligursky marked this pull request as draft February 4, 2022 12:03
@mvaligursky mvaligursky marked this pull request as ready for review February 4, 2022 12:34
Copy link
Member

@slimbuck slimbuck left a comment

Choose a reason for hiding this comment

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

Could you perhaps give us a summary of the planned class structure we'll end up with?

src/graphics/graphics-device.js Outdated Show resolved Hide resolved
src/graphics/graphics-device.js Show resolved Hide resolved
@mvaligursky mvaligursky merged commit 48a7097 into dev Feb 7, 2022
@mvaligursky mvaligursky deleted the mvaligursky-webgl-vb-ib branch February 7, 2022 09:32
@willeastcott
Copy link
Contributor

I'm OK enough with this to approve it. But I still have a few thoughts swirling in my head:

  1. What can we do to address class cyclic dependencies. Right now the design reinforces this cycle between the Device and the Buffer classes. For example, the WebGL buffer must modify state on the device when it's destroyed. Could that be an event? Or could the global state of the current bound buffer be managed in the WebGLVerterBuffer class?
  2. I'm still not sure how we plan to ship a WebGPU flavor to devs. Do we pursue the 'all in one' approach or have two engine versions?
  3. Are we concerned about lots of inheritance on performance?
  4. I'm a bit undecided on naming. The WebGL spec itself already defined names like WebGLTexture and WebGLBuffer. I do like keeping the WebGL style of capitalization. But the API itself uses some of these names already and we'll probably cause problems if we reuse them. Yeah, just pointing out I'm still thinking about that. Obviously, these classes are private so not a huge deal.

@mvaligursky
Copy link
Contributor Author

I'm OK enough with this to approve it. But I still have a few thoughts swirling in my head

  1. I don't see a problem with classes on the same level in the architecture (in this case inside graphics) calling each other - this is a common practise in js, c#, c++ - it does not create a circular dependency, as only type for type-checking needs to be imported.

  2. I see two main directions here:
    A) (ideal) We work out this PR (Lite version of the Application to allow for tree-shaking (WIP) #3949) plus follow up PRs, related to creating a lite version of the application. This would allow the device creation to be moved from the Application class itself, into a user code. And so the user could import only WebglGraphicsDevice, construct an instance and assign it to the ApplicationLite. Or they create only Webpu version, or both based on support of the device. And the tree-shaking will only add those modules to the user app during the build.
    B) (backup) We ship multiple builds of the engine. Most likely "webgl" and "webgl + webgpu" flavour initially.

  3. Only slightly, but I thought about it as well. Most of the functionality requiring calls to various parts of the inheritance chain would be limited to the resource creation / updates, which is not frequent. The hot path, where the resource is used for rendering would usually only access properties directly in the Impl classes. But we'll need to keep an eye on this for sure.

  4. We could also adopt some shorter names, perhaps:

  • WglTexture and WgpuTexture, or only GlTexture and GpuTexture, even though this one is likely confusing.
  • Or change the order ... so TextureWebGl, TextureWebGpu - I'm pretty keen to this one.

@mvaligursky mvaligursky mentioned this pull request Jul 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: graphics Graphics related issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants