Refactor 2d render pipeline #1708
Conversation
| for (let i = 0; i < charCount; ++i) { | ||
| const charRenderData = charRenderDatas[i]; | ||
| const spriteRenderData = spriteRenderDataPool.getFromPool(); | ||
| spriteRenderData.set(this, material, charRenderData.renderData, charRenderData.texture, i); | ||
| charsData[i] = spriteRenderData; | ||
| spriteRenderData.set(this, material, charRenderData.renderData, charRenderData.texture); |
There was a problem hiding this comment.
charRenderData.usage = RenderDataUsage.Sprite;
packages/core/src/Camera.ts
Outdated
| @@ -70,6 +71,9 @@ export class Camera extends Component { | |||
| _renderPipeline: BasicRenderPipeline; | |||
| /** @internal */ | |||
| @ignoreClone | |||
| _batcherManager: BatcherManager; | |||
There was a problem hiding this comment.
Why not in BasicRenderPipeline.
| } | ||
|
|
||
| commitRenderData(context: RenderContext, data: RenderData | Array<RenderData>): void { | ||
| if (!data) return; |
There was a problem hiding this comment.
This line can be deleted.
| /** @internal */ | ||
| _meshCount: number = 1; | ||
| /** @internal */ | ||
| _subMeshPool: ClassPool<SubMesh> = new ClassPool(SubMesh); |
There was a problem hiding this comment.
Can be a static variable.
There was a problem hiding this comment.
Can be a static variable.
has deleted
|
|
||
| commitRenderData(context: RenderContext, data: SpriteRenderData): void { | ||
| if (this._preSpriteRenderData) { | ||
| if (this._vertexCount + data.verticesData.vertexCount > Batcher2D.MAX_VERTEX_COUNT) { |
There was a problem hiding this comment.
Need to judge index
|
There is a doubt that the current |
| this._cascadedShadowCaster = new CascadedShadowCasterPass(camera); | ||
| this._depthOnlyPass = new DepthOnlyPass(engine); | ||
| this._spriteMaskManager = new SpriteMaskManager(camera.engine); |
There was a problem hiding this comment.
Is it better to put SpriteMaskManager in Scene? Because :
- Mask does not depend on rendering timing.
- More memory efficient.
| /** @internal */ | ||
| _componentInstanceId: number; | ||
| /** @internal */ | ||
| _distanceForSort: number; |
There was a problem hiding this comment.
This is a internal class, why use internal property?
|
|
||
| /** | ||
| * Render queue. | ||
| */ | ||
| export class RenderQueue { | ||
| private static _textureProperty: ShaderProperty = ShaderProperty.getByName("renderer_SpriteTexture"); | ||
|
|
There was a problem hiding this comment.
you define a render property in Renderqueue?
There was a problem hiding this comment.
Just for text that with multi-texture, i do not have any good idea now
| } else { | ||
| return distanceDiff; | ||
| } | ||
| return dataA._distanceForSort - dataB._distanceForSort || instanceIdDiff; |
There was a problem hiding this comment.
This is not OK?
return (
dataA._priority - dataB._priority ||
dataA._distanceForSort - dataB._distanceForSort ||
dataA._componentInstanceId - dataB._componentInstanceId ||
dataA._materialPriority - dataB._materialPriority
);There was a problem hiding this comment.
Yeah, in some case, the same renderer wo want control the sort just for material priority, link lottie or spine
| // TextRenderer may be has multi-texture. | ||
| const isText = usage === RenderDataUsage.Text; | ||
| // @ts-ignore | ||
| isText && rendererData.setTexture(RenderQueue._textureProperty, data.texture); |
There was a problem hiding this comment.
No!!!!! This configuration should be done at the upper level
There was a problem hiding this comment.
I don't has a good idea how to handle a text renderer with multi-texture right now
| const { usage } = data; | ||
| const needMask = usage === RenderDataUsage.Sprite || usage === RenderDataUsage.Text; | ||
| const renderer = data.component; | ||
| needMask && spriteMaskManager.preRender(camera, <SpriteRenderer>renderer); |
| compileMacros | ||
| ); | ||
| const { usage } = data; | ||
| const needMask = usage === RenderDataUsage.Sprite || usage === RenderDataUsage.Text; |
There was a problem hiding this comment.
SpriteMask is still very specialized!
| import { SpriteMaskBatcher } from "./SpriteMaskBatcher"; | ||
| import { SpriteMaskRenderData } from "./SpriteMaskRenderData"; | ||
| import { StencilOperation } from "../shader"; | ||
| import { SpriteMaskBatcher } from "./batcher/SpriteMaskBatcher"; |
| } | ||
|
|
||
| set( | ||
| override set( |
There was a problem hiding this comment.
Further abstraction is needed. In addition, this is a common class between text and elves. The current definition is unreasonable.
| import { RenderDataUsage } from "../enums/RenderDataUsage"; | ||
| import { Batcher2D } from "./Batcher2D"; | ||
|
|
||
| export class BatcherManager { |
There was a problem hiding this comment.
What is the value of this class?
There was a problem hiding this comment.
future, i want handle all batch logic in batch function
| /** | ||
| * @internal | ||
| */ | ||
| export class MeshBuffer { |
There was a problem hiding this comment.
Vague definition, and needs to be redesigned！！！！！！！！！
| import { SpriteRenderData } from "../SpriteRenderData"; | ||
| import { Batcher2D } from "./Batcher2D"; | ||
|
|
||
| export class SpriteMaskBatcher extends Batcher2D { |
There was a problem hiding this comment.
Basically no design, may need to be rewritten
| */ | ||
| export class MeshBuffer { | ||
| /** @internal */ | ||
| _mesh: BufferMesh; |
| * @internal | ||
| */ | ||
| protected _canBatch(elementA: RenderElement, elementB: RenderElement): boolean { | ||
| return false; |
There was a problem hiding this comment.
It's better to use RenderData, RenderElement is specific to RenderQueue
| * @internal | ||
| */ | ||
| protected _batchRenderElement(elementA: RenderElement, elementB?: RenderElement): void {} | ||
|
|
There was a problem hiding this comment.
It's better to use RenderData, RenderElement is specific to RenderQueue
galacean-refactor-2d-render-pipeline.zip