-
-
Notifications
You must be signed in to change notification settings - Fork 300
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
Refactor mask #2369
base: dev/1.4
Are you sure you want to change the base?
Refactor mask #2369
Changes from 31 commits
3ff8a7b
4afe44b
603ba2a
0ae6484
1a89b5d
a5f1ab9
7ad02b4
e97c4aa
2f2615b
47c9c7e
e52b60a
8a48797
a311f2d
9a06859
f6f373b
f6b35a9
47d670a
8c0fc37
e71b5d6
5df85bc
0a09fee
82f7f82
12d40f3
028e4b0
2b92fe1
3d10522
5f50e58
f96f891
ea9e05c
6e6eab5
ce3efed
bd38b58
18ea656
894dff9
f0c6d4b
4f4218d
397ac0e
d29b823
8a34428
61558b1
3fd4e4f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,11 +9,14 @@ import { SubPrimitiveChunk } from "../../RenderPipeline/SubPrimitiveChunk"; | |
import { SubRenderElement } from "../../RenderPipeline/SubRenderElement"; | ||
import { Renderer, RendererUpdateFlags } from "../../Renderer"; | ||
import { assignmentClone, ignoreClone } from "../../clone/CloneManager"; | ||
import { SpriteMaskLayer } from "../../enums/SpriteMaskLayer"; | ||
import { ShaderProperty } from "../../shader/ShaderProperty"; | ||
import { SimpleSpriteAssembler } from "../assembler/SimpleSpriteAssembler"; | ||
import { SpriteMaskLayer } from "../enums/SpriteMaskLayer"; | ||
import { SpriteModifyFlags } from "../enums/SpriteModifyFlags"; | ||
import { Sprite } from "./Sprite"; | ||
import { Material } from "../../material"; | ||
import { ColorWriteMask, CullMode, Shader } from "../../shader"; | ||
import { Engine } from "../../Engine"; | ||
|
||
/** | ||
* A component for masking Sprites. | ||
|
@@ -24,6 +27,18 @@ export class SpriteMask extends Renderer { | |
/** @internal */ | ||
static _alphaCutoffProperty: ShaderProperty = ShaderProperty.getByName("renderer_MaskAlphaCutoff"); | ||
|
||
/** @internal */ | ||
static _createSpriteMaskMaterial(engine: Engine): Material { | ||
const material = new Material(engine, Shader.find("SpriteMask")); | ||
const renderState = material.renderState; | ||
renderState.blendState.targetBlendState.colorWriteMask = ColorWriteMask.None; | ||
renderState.rasterState.cullMode = CullMode.Off; | ||
renderState.stencilState.enabled = true; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe don't need! |
||
renderState.depthState.enabled = false; | ||
material.isGCIgnored = true; | ||
return material; | ||
} | ||
|
||
/** The mask layers the sprite mask influence to. */ | ||
@assignmentClone | ||
influenceLayers: number = SpriteMaskLayer.Everything; | ||
|
Original file line number | Diff line number | Diff line change | ||||||
---|---|---|---|---|---|---|---|---|
|
@@ -8,7 +8,6 @@ | |||||||
import { Renderer, RendererUpdateFlags } from "../../Renderer"; | ||||||||
import { assignmentClone, deepClone, ignoreClone } from "../../clone/CloneManager"; | ||||||||
import { ShaderProperty } from "../../shader/ShaderProperty"; | ||||||||
import { CompareFunction } from "../../shader/enums/CompareFunction"; | ||||||||
import { ISpriteAssembler } from "../assembler/ISpriteAssembler"; | ||||||||
import { SimpleSpriteAssembler } from "../assembler/SimpleSpriteAssembler"; | ||||||||
import { SlicedSpriteAssembler } from "../assembler/SlicedSpriteAssembler"; | ||||||||
|
@@ -18,6 +17,8 @@ | |||||||
import { SpriteModifyFlags } from "../enums/SpriteModifyFlags"; | ||||||||
import { SpriteTileMode } from "../enums/SpriteTileMode"; | ||||||||
import { Sprite } from "./Sprite"; | ||||||||
import { Material } from "../../material"; | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add missing import for The Apply this diff to add the missing import: import { Material } from "../../material";
+import { Engine } from "../../Engine";
import { BlendFactor, BlendOperation, CullMode, RenderQueueType, Shader } from "../../shader"; 📝 Committable suggestion
Suggested change
|
||||||||
import { BlendFactor, BlendOperation, CullMode, RenderQueueType, Shader } from "../../shader"; | ||||||||
|
||||||||
/** | ||||||||
* Renders a Sprite for 2D graphics. | ||||||||
|
@@ -26,6 +27,24 @@ | |||||||
/** @internal */ | ||||||||
static _textureProperty: ShaderProperty = ShaderProperty.getByName("renderer_SpriteTexture"); | ||||||||
|
||||||||
/** @internal */ | ||||||||
static _createSpriteMaterial(engine): Material { | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Specify type annotation for The Apply this diff to specify the type annotation: -static _createSpriteMaterial(engine): Material {
+static _createSpriteMaterial(engine: Engine): Material { 📝 Committable suggestion
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Migrate to |
||||||||
const material = new Material(engine, Shader.find("Sprite")); | ||||||||
const renderState = material.renderState; | ||||||||
const target = renderState.blendState.targetBlendState; | ||||||||
target.enabled = true; | ||||||||
target.sourceColorBlendFactor = BlendFactor.SourceAlpha; | ||||||||
target.destinationColorBlendFactor = BlendFactor.OneMinusSourceAlpha; | ||||||||
target.sourceAlphaBlendFactor = BlendFactor.One; | ||||||||
target.destinationAlphaBlendFactor = BlendFactor.OneMinusSourceAlpha; | ||||||||
target.colorBlendOperation = target.alphaBlendOperation = BlendOperation.Add; | ||||||||
renderState.depthState.writeEnabled = false; | ||||||||
renderState.rasterState.cullMode = CullMode.Off; | ||||||||
renderState.renderQueueType = RenderQueueType.Transparent; | ||||||||
material.isGCIgnored = true; | ||||||||
return material; | ||||||||
} | ||||||||
|
||||||||
/** @internal */ | ||||||||
@ignoreClone | ||||||||
_subChunk: SubPrimitiveChunk; | ||||||||
|
@@ -257,7 +276,6 @@ | |||||||
|
||||||||
set maskInteraction(value: SpriteMaskInteraction) { | ||||||||
if (this._maskInteraction !== value) { | ||||||||
this._updateStencilState(this._maskInteraction, value); | ||||||||
this._maskInteraction = value; | ||||||||
} | ||||||||
} | ||||||||
|
@@ -334,7 +352,7 @@ | |||||||
} | ||||||||
// @todo: This question needs to be raised rather than hidden. | ||||||||
if (material.destroyed) { | ||||||||
material = this._engine._spriteDefaultMaterials[this._maskInteraction]; | ||||||||
material = this._engine._spriteDefaultMaterial; | ||||||||
} | ||||||||
|
||||||||
// Update position | ||||||||
|
@@ -395,28 +413,6 @@ | |||||||
this._dirtyUpdateFlag &= ~SpriteRendererUpdateFlags.AutomaticSize; | ||||||||
} | ||||||||
|
||||||||
private _updateStencilState(from: SpriteMaskInteraction, to: SpriteMaskInteraction): void { | ||||||||
const material = this.getMaterial(); | ||||||||
const { _spriteDefaultMaterials: spriteDefaultMaterials } = this._engine; | ||||||||
if (material === spriteDefaultMaterials[from]) { | ||||||||
this.setMaterial(spriteDefaultMaterials[to]); | ||||||||
} else { | ||||||||
const { stencilState } = material.renderState; | ||||||||
if (to === SpriteMaskInteraction.None) { | ||||||||
stencilState.enabled = false; | ||||||||
stencilState.writeMask = 0xff; | ||||||||
stencilState.referenceValue = 0; | ||||||||
stencilState.compareFunctionFront = stencilState.compareFunctionBack = CompareFunction.Always; | ||||||||
} else { | ||||||||
stencilState.enabled = true; | ||||||||
stencilState.writeMask = 0x00; | ||||||||
stencilState.referenceValue = 1; | ||||||||
stencilState.compareFunctionFront = stencilState.compareFunctionBack = | ||||||||
to === SpriteMaskInteraction.VisibleInsideMask ? CompareFunction.LessEqual : CompareFunction.Greater; | ||||||||
} | ||||||||
} | ||||||||
} | ||||||||
|
||||||||
@ignoreClone | ||||||||
private _onSpriteChange(type: SpriteModifyFlags): void { | ||||||||
switch (type) { | ||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,8 +9,15 @@ import { SubRenderElement } from "../../RenderPipeline/SubRenderElement"; | |
import { Renderer } from "../../Renderer"; | ||
import { TransformModifyFlags } from "../../Transform"; | ||
import { assignmentClone, deepClone, ignoreClone } from "../../clone/CloneManager"; | ||
import { ShaderData, ShaderProperty } from "../../shader"; | ||
import { CompareFunction } from "../../shader/enums/CompareFunction"; | ||
import { | ||
BlendFactor, | ||
BlendOperation, | ||
CullMode, | ||
RenderQueueType, | ||
Shader, | ||
ShaderData, | ||
ShaderProperty | ||
} from "../../shader"; | ||
import { ShaderDataGroup } from "../../shader/enums/ShaderDataGroup"; | ||
import { Texture2D } from "../../texture"; | ||
import { FontStyle } from "../enums/FontStyle"; | ||
|
@@ -21,6 +28,7 @@ import { CharRenderInfo } from "./CharRenderInfo"; | |
import { Font } from "./Font"; | ||
import { SubFont } from "./SubFont"; | ||
import { TextUtils } from "./TextUtils"; | ||
import { Material } from "../../material"; | ||
|
||
/** | ||
* Renders a text for 2D graphics. | ||
|
@@ -32,6 +40,24 @@ export class TextRenderer extends Renderer { | |
private static _worldPositions = [new Vector3(), new Vector3(), new Vector3(), new Vector3()]; | ||
private static _charRenderInfos: CharRenderInfo[] = []; | ||
|
||
/** @internal */ | ||
static _createTextMaterial(engine: Engine): Material { | ||
const material = new Material(engine, Shader.find("Text")); | ||
const renderState = material.renderState; | ||
const target = renderState.blendState.targetBlendState; | ||
target.enabled = true; | ||
target.sourceColorBlendFactor = BlendFactor.SourceAlpha; | ||
target.destinationColorBlendFactor = BlendFactor.OneMinusSourceAlpha; | ||
target.sourceAlphaBlendFactor = BlendFactor.One; | ||
target.destinationAlphaBlendFactor = BlendFactor.OneMinusSourceAlpha; | ||
target.colorBlendOperation = target.alphaBlendOperation = BlendOperation.Add; | ||
renderState.depthState.writeEnabled = false; | ||
renderState.rasterState.cullMode = CullMode.Off; | ||
renderState.renderQueueType = RenderQueueType.Transparent; | ||
material.isGCIgnored = true; | ||
return material; | ||
} | ||
|
||
/** @internal */ | ||
@ignoreClone | ||
_textChunks = Array<TextChunk>(); | ||
|
@@ -250,7 +276,6 @@ export class TextRenderer extends Renderer { | |
set maskInteraction(value: SpriteMaskInteraction) { | ||
if (this._maskInteraction !== value) { | ||
this._maskInteraction = value; | ||
this._setDirtyFlagTrue(DirtyFlag.MaskInteraction); | ||
} | ||
} | ||
|
||
|
@@ -394,11 +419,6 @@ export class TextRenderer extends Renderer { | |
return; | ||
} | ||
|
||
if (this._isContainDirtyFlag(DirtyFlag.MaskInteraction)) { | ||
this._updateStencilState(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Before implement is different! |
||
this._setDirtyFlagFalse(DirtyFlag.MaskInteraction); | ||
} | ||
|
||
if (this._isContainDirtyFlag(DirtyFlag.SubFont)) { | ||
this._resetSubFont(); | ||
this._setDirtyFlagFalse(DirtyFlag.SubFont); | ||
|
@@ -437,29 +457,6 @@ export class TextRenderer extends Renderer { | |
camera._renderPipeline.pushRenderElement(context, renderElement); | ||
} | ||
|
||
private _updateStencilState(): void { | ||
const material = this.getInstanceMaterial(); | ||
const stencilState = material.renderState.stencilState; | ||
const maskInteraction = this._maskInteraction; | ||
|
||
if (maskInteraction === SpriteMaskInteraction.None) { | ||
stencilState.enabled = false; | ||
stencilState.writeMask = 0xff; | ||
stencilState.referenceValue = 0; | ||
stencilState.compareFunctionFront = stencilState.compareFunctionBack = CompareFunction.Always; | ||
} else { | ||
stencilState.enabled = true; | ||
stencilState.writeMask = 0x00; | ||
stencilState.referenceValue = 1; | ||
const compare = | ||
maskInteraction === SpriteMaskInteraction.VisibleInsideMask | ||
? CompareFunction.LessEqual | ||
: CompareFunction.Greater; | ||
stencilState.compareFunctionFront = compare; | ||
stencilState.compareFunctionBack = compare; | ||
} | ||
} | ||
|
||
private _resetSubFont(): void { | ||
const font = this._font; | ||
this._subFont = font._getSubFont(this.fontSize, this.fontStyle); | ||
|
@@ -762,8 +759,7 @@ enum DirtyFlag { | |
LocalPositionBounds = 0x2, | ||
WorldPosition = 0x4, | ||
WorldBounds = 0x8, | ||
MaskInteraction = 0x10, | ||
Color = 0x20, | ||
Color = 0x10, | ||
|
||
Position = LocalPositionBounds | WorldPosition | WorldBounds, | ||
Font = SubFont | Position | ||
Comment on lines
+735
to
738
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Codebase verification Incomplete Removal of The search revealed multiple remaining references to
Please address these remaining references to ensure the 🔗 Analysis chainVerify the impact of DirtyFlag enum changes. The Please ensure that:
🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Search for any remaining references to DirtyFlag.MaskInteraction
echo "Searching for DirtyFlag.MaskInteraction references:"
rg "DirtyFlag\.MaskInteraction" --type ts
# Search for uses of DirtyFlag.Color to ensure it's being used correctly
echo "Searching for DirtyFlag.Color usage:"
rg "DirtyFlag\.Color" --type ts
Length of output: 41441 |
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -166,7 +166,7 @@ export class BasicRenderPipeline { | |
const clearFlags = camera.clearFlags & ~(ignoreClear ?? CameraClearFlags.None); | ||
const color = background.solidColor; | ||
if (clearFlags !== CameraClearFlags.None) { | ||
rhi.clearRenderTarget(camera.engine, clearFlags, color); | ||
rhi.clearRenderTarget(engine, clearFlags, color); | ||
} | ||
|
||
opaqueQueue.render(context, PipelineStage.Forward); | ||
|
@@ -195,6 +195,9 @@ export class BasicRenderPipeline { | |
} | ||
|
||
transparentQueue.render(context, PipelineStage.Forward); | ||
// Reset stencil | ||
scene._maskManager.clearMask(context, PipelineStage.Forward); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why clear here? |
||
scene._stencilManager.clearStencil(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Codebase verification Stencil state manipulations found outside of StencilManager The verification process revealed several instances where stencil state operations are performed outside of the Please refactor these instances to ensure all stencil operations are handled through 🔗 Analysis chainVerify stencil reset and mask clearing The addition of stencil reset and mask clearing after rendering the transparent queue is a good step towards addressing the concerns raised in the PR comments. This change should help manage stencil states more effectively. To ensure this change resolves the issues mentioned in the PR comments, please run the following verification: These tests will help ensure that the stencil and mask management is now centralized and consistent across the codebase. 🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Description: Check for any remaining stencil or mask-related issues after these changes
# Test 1: Search for any remaining stencil-related TODOs or FIXMEs
echo "Searching for stencil-related TODOs or FIXMEs:"
rg -i "TODO|FIXME" -g "*.ts" | rg -i "stencil"
# Test 2: Search for any remaining mask-related TODOs or FIXMEs
echo "Searching for mask-related TODOs or FIXMEs:"
rg -i "TODO|FIXME" -g "*.ts" | rg -i "mask"
# Test 3: Check for any direct stencil state manipulations outside of StencilManager
echo "Checking for direct stencil state manipulations:"
rg "stencil" -g "*.ts" | rg -v "StencilManager"
# Test 4: Verify that MaskManager is used consistently
echo "Verifying consistent use of MaskManager:"
rg "MaskManager" -g "*.ts"
Length of output: 17524 |
||
|
||
const postProcessManager = scene._postProcessManager; | ||
const cameraRenderTarget = camera.renderTarget; | ||
|
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.
💡 Codebase verification
Residual old import paths detected
The script found an occurrence of the old import path in
packages/core/src/RenderPipeline/MaskManager.ts
. Please update this reference to ensure consistency across the codebase.🔗 Analysis chain
Verify import path consistency across the codebase
The import path for
SpriteMaskLayer
has been updated. Ensure that this change is consistent across the entire codebase to prevent any import-related issues.Run the following script to check for any remaining occurrences of the old import path:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
Length of output: 178
Script:
Length of output: 156