-
-
Notifications
You must be signed in to change notification settings - Fork 310
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
CamerasetReplacementShader
support ReplacementFailureStrategy
when failed
#2163
CamerasetReplacementShader
support ReplacementFailureStrategy
when failed
#2163
Conversation
WalkthroughThe recent changes focus on enhancing shader management within the rendering pipeline. A significant addition is the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
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.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- packages/core/src/RenderPipeline/BasicRenderPipeline.ts (1 hunks)
Additional comments not posted (4)
packages/core/src/RenderPipeline/BasicRenderPipeline.ts (4)
214-214
: Ensure the correctness ofmaterialSubShaderTagValue
.The
materialSubShaderTagValue
is correctly obtained from thematerialSubShader
using thegetTagValue
method. This ensures that the tag value is properly fetched for comparison in subsequent logic.
215-215
: Initializepasses
variable to default shader passes.The
passes
variable is initialized to the default shader passes frommaterialSubShader
. This ensures that if no replacement shader matches, the default shader passes are used.
218-219
: Updatepasses
variable if replacement shader matches.The loop iterates through the
replacementSubShaders
to find a matching tag value. If a match is found, thepasses
variable is updated to use the matching subshader's passes. This ensures that the correct shader passes are used based on the tag value.
223-223
: Utilize the updatedpasses
variable inpushRenderDataWithShader
.The
passes
variable, which may have been updated based on the replacement shader, is passed to thepushRenderDataWithShader
method. This ensures that the correct shader passes are used during rendering.
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.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- packages/core/src/RenderPipeline/BasicRenderPipeline.ts (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- packages/core/src/RenderPipeline/BasicRenderPipeline.ts
API design to solve this need: /**
* Set the replacement shader.
* @param shader - Replacement shader
* @param replacementTagName - Sub shader tag name
* @param failureStrategy - Replacement failure strategy, @defaultValue `ReplacementFailureStrategy.KeepOriginalShader`
*
* @remarks
* If replacementTagName is not specified, the first sub shader will be replaced.
* If replacementTagName is specified, the replacement shader will find the first sub shader which has the same tag value get by replacementTagKey. If failed to find the sub shader, the strategy will be determined by failureStrategy.
*/
setReplacementShader(shader: Shader, replacementTagName?: string, failureStrategy?: ReplacementFailureStrategy): void;
/**
* Set the replacement shader.
* @param shader - Replacement shader
* @param replacementTag - Sub shader tag
* @param failureStrategy - Replacement failure strategy, @defaultValue `ReplacementFailureStrategy.KeepOriginalShader`
*
* @remarks
* If replacementTag is not specified, the first sub shader will be replaced.
* If replacementTag is specified, the replacement shader will find the first sub shader which has the same tag value get by replacementTagKey. If failed to find the sub shader, the strategy will be determined by failureStrategy.
*/
setReplacementShader(shader: Shader, replacementTag?: ShaderTagKey, failureStrategy?: ReplacementFailureStrategy);
setReplacementShader(
shader: Shader,
replacementTag?: string | ShaderTagKey,
failureStrategy: ReplacementFailureStrategy = ReplacementFailureStrategy.KeepOriginalShader
): void {
this._replacementShader = shader;
this._replacementSubShaderTag =
typeof replacementTag === "string" ? ShaderTagKey.getByName(replacementTag) : replacementTag;
// @todo: failureStrategy
} /**
* Replacement failure strategy.
*/
export enum ReplacementFailureStrategy {
/** Keep the original shader. */
KeepOriginalShader,
/** Do not render. */
DoNotRender
} |
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.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (6)
- packages/core/src/Camera.ts (5 hunks)
- packages/core/src/RenderPipeline/BasicRenderPipeline.ts (2 hunks)
- packages/core/src/RenderPipeline/RenderContext.ts (2 hunks)
- packages/core/src/enums/ReplacementFailureStrategy.ts (1 hunks)
- packages/core/src/index.ts (1 hunks)
- tests/src/core/Camera.test.ts (2 hunks)
Files skipped from review due to trivial changes (1)
- packages/core/src/enums/ReplacementFailureStrategy.ts
Files skipped from review as they are similar to previous changes (1)
- packages/core/src/RenderPipeline/BasicRenderPipeline.ts
Additional comments not posted (10)
packages/core/src/RenderPipeline/RenderContext.ts (2)
4-4
: Import statement added forReplacementFailureStrategy
.The import statement is correctly added to bring in the
ReplacementFailureStrategy
enum.
32-32
: New public propertyreplacementFailureStrategy
added.The new property aligns with the changes to incorporate shader replacement failure strategies.
packages/core/src/index.ts (1)
47-47
: Export statement added forReplacementFailureStrategy
.The export statement is correctly added to make the
ReplacementFailureStrategy
enum available for use in other parts of the application.tests/src/core/Camera.test.ts (3)
1-1
: Import statements added forReplacementFailureStrategy
andShader
.The import statements are correctly added to bring in the necessary entities for the new test cases.
81-91
: Test shaders defined for replacement shader tests.The vertex and fragment shaders are defined correctly for use in the replacement shader tests.
92-108
: Test cases added forsetReplacementShader
method.The test cases correctly cover the scenarios for setting and resetting replacement shaders using the
ReplacementFailureStrategy
.packages/core/src/Camera.ts (4)
18-18
: Import statement added forReplacementFailureStrategy
.The import statement is correctly added to bring in the
ReplacementFailureStrategy
enum.
107-107
: New private property_replacementFailureStrategy
added.The new property aligns with the changes to incorporate shader replacement failure strategies.
623-651
: UpdatedsetReplacementShader
method to handlefailureStrategy
parameter.The method is correctly updated to handle the
failureStrategy
parameter and set the appropriate private properties.
660-660
: UpdatedresetReplacementShader
method to clear_replacementFailureStrategy
.The method is correctly updated to clear the
_replacementFailureStrategy
property.
camera.setReplacementShader
specifies tag
causing rendering exceptionsetReplacementShader
support ReplacementFailureStrategy
when failed
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.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- packages/core/src/index.ts (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- packages/core/src/index.ts
Demo:
Summary by CodeRabbit
New Features
ReplacementFailureStrategy
enum for handling shader replacement failures.Camera
class with new shader replacement strategies.Bug Fixes
BasicRenderPipeline
to handle tags and passes correctly.Tests
Camera
functionality.Chores