-
Notifications
You must be signed in to change notification settings - Fork 32
Refactor/remove render ops #727
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
base: main
Are you sure you want to change the base?
Conversation
| bindRenderOp(renderOp: WebGlRenderOp) { | ||
| this.bindBufferCollection(renderOp.buffers); | ||
| this.bindTextures(renderOp.textures); | ||
| const isCoreNode = renderOp instanceof CoreNode; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This instanceof check is a bit expensive since there a lot to check in CoreNode. This needs to be replaced by a simpler check
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
| return (this.stage.renderer as WebGlRenderer).quadBufferCollection; | ||
| } | ||
|
|
||
| get width(): number { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't need these, use get w if you really must, but if its for internal use, avoid using getters and just use props.w / props.h
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a lot of overhead. With the change to w / h it wasn't done throughout the code base. So now we face w => width and vice versa and same with height. That requires if statements in a lot of places. If you and the team want to spend the time to make it consistent through out the code base that would allow removing this.
| return this.textureCoords || this.stage.renderer.defaultTextureCoords; | ||
| } | ||
|
|
||
| get quadBufferCollection(): BufferCollection { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is very WebGl specific, handle this in WebGLRenderer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CoreNodes are now renderOps.
| return this.props.texture || this.stage.defaultTexture; | ||
| } | ||
|
|
||
| get renderTextureCoords(): TextureCoords | undefined { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is very WebGl specific, handle this in WebGLRenderer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Canvas renderer needs this as well
Phase 2 - after #726
Removes RenderOps and leaves it on CoreNode. Cleanup property differences between SDF & CoreNode