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

Fix clear flag of internal render target #2444

Merged
merged 19 commits into from
Dec 2, 2024
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 17 additions & 8 deletions e2e/case/.mockForE2E.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ let flipYCanvas: HTMLCanvasElement = null;

export function initScreenshot(
engine: Engine,
camera: Camera,
camera: Camera | Camera[],
width: number = 1200,
height: number = 800,
flipY = false,
Expand All @@ -38,15 +38,25 @@ export function initScreenshot(
const isPaused = engine.isPaused;
engine.pause();

const originalTarget = camera.renderTarget;
const cameras = Array.isArray(camera) ? camera : [camera];
const callbacks = [];
const renderColorTexture = new Texture2D(engine, width, height);
const renderTargetData = new Uint8Array(width * height * 4);
const renderTarget = new RenderTarget(engine, width, height, renderColorTexture, undefined, 1);

// render to off-screen
camera.renderTarget = renderTarget;
camera.aspectRatio = width / height;
camera.render();
cameras.forEach((camera) => {
const originalTarget = camera.renderTarget;

// render to off-screen
camera.renderTarget = renderTarget;
camera.aspectRatio = width / height;
camera.render();

callbacks.push(() => {
camera.renderTarget = originalTarget;
camera.resetAspectRatio();
});
});

renderColorTexture.getPixelBuffer(0, 0, width, height, 0, renderTargetData);

Expand Down Expand Up @@ -94,8 +104,7 @@ export function initScreenshot(
// window.URL.revokeObjectURL(url);

// revert
camera.renderTarget = originalTarget;
camera.resetAspectRatio();
callbacks.forEach((cb) => cb());
!isPaused && engine.resume();
},
isPNG ? "image/png" : "image/jpeg",
Expand Down
83 changes: 83 additions & 0 deletions e2e/case/multi-camera-no-clear.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,83 @@
/**
* @title Multi camera no clear
* @category Advance
*/
import {
BlinnPhongMaterial,
Camera,
CameraClearFlags,
Color,
Engine,
Layer,
Logger,
MeshRenderer,
PrimitiveMesh,
Scene,
WebGLEngine
} from "@galacean/engine";
import { initScreenshot, updateForE2E } from "./.mockForE2E";

Logger.enable();
WebGLEngine.create({ canvas: "canvas" }).then((engine) => {
engine.canvas.resizeByClientSize();

initFirstScene(engine);
engine.run();
});
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add error handling and cleanup

The engine creation promise lacks error handling and resource cleanup. Consider adding:

  1. Error handling for engine creation
  2. Proper cleanup of resources after the test
-WebGLEngine.create({ canvas: "canvas" }).then((engine) => {
+WebGLEngine.create({ canvas: "canvas" })
+  .then((engine) => {
   engine.canvas.resizeByClientSize();
   initFirstScene(engine);
   engine.run();
+    return engine;
+  })
+  .catch((error) => {
+    console.error("Failed to create engine:", error);
+    throw error;
+  })
+  .finally(() => {
+    // Cleanup resources
+  });

Committable suggestion skipped: line range outside the PR's diff.


function initFirstScene(engine: Engine): Scene {
const scene = engine.sceneManager.scenes[0];
const rootEntity = scene.createRootEntity();

// Create full screen camera
const cameraEntity = rootEntity.createChild("fullscreen-camera");
const camera = cameraEntity.addComponent(Camera);
camera.cullingMask = Layer.Layer0;
cameraEntity.transform.setPosition(0, 0, 20);

// Create cube
const cubeEntity = rootEntity.createChild("cube");
cubeEntity.transform.setPosition(-3, 0, 3);
const renderer = cubeEntity.addComponent(MeshRenderer);
renderer.mesh = PrimitiveMesh.createSphere(engine, 2, 24);
const material = new BlinnPhongMaterial(engine);
material.baseColor = new Color(1, 0, 0, 1);
renderer.setMaterial(material);

{
const cameraEntity = rootEntity.createChild("window-camera");
const camera2 = cameraEntity.addComponent(Camera);
camera2.cullingMask = Layer.Layer1;
camera2.enablePostProcess = true;
camera2.enableHDR = true;
camera2.clearFlags = CameraClearFlags.None;
// camera.msaaSamples = 4;

// @ts-ignore
const bloomEffect = scene._postProcessManager._bloomEffect;
// @ts-ignore
const tonemappingEffect = scene._postProcessManager._tonemappingEffect;
GuoLei1990 marked this conversation as resolved.
Show resolved Hide resolved

bloomEffect.enabled = true;
tonemappingEffect.enabled = true;
bloomEffect.threshold = 0.1;
bloomEffect.intensity = 2;
cameraEntity.transform.setPosition(0, 0, 20);

// Create cube
const cubeEntity = rootEntity.createChild("cube");
cubeEntity.layer = Layer.Layer1;
cubeEntity.transform.setPosition(-2, 0, 3);
const renderer = cubeEntity.addComponent(MeshRenderer);
renderer.mesh = PrimitiveMesh.createSphere(engine, 2, 24);
const material = new BlinnPhongMaterial(engine);
material.baseColor = new Color(1, 0, 0, 1);
material.emissiveColor.set(1, 0, 0, 1);
renderer.setMaterial(material);

updateForE2E(engine);
initScreenshot(engine, [camera, camera2]);
}

return scene;
}
85 changes: 85 additions & 0 deletions e2e/case/multi-scene-clear.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,85 @@
/**
* @title Multi scene clear
* @category Advance
*/
import {
BlinnPhongMaterial,
Camera,
CameraClearFlags,
Color,
Engine,
Logger,
MeshRenderer,
PrimitiveMesh,
Scene,
WebGLEngine
} from "@galacean/engine";
import { initScreenshot, updateForE2E } from "./.mockForE2E";

Logger.enable();
WebGLEngine.create({ canvas: "canvas" }).then((engine) => {
engine.canvas.resizeByClientSize();

const firstCamera = initFirstScene(engine);
const secondCamera = initSecondScene(engine);

updateForE2E(engine);
initScreenshot(engine, [firstCamera, secondCamera]);
// initScreenshot(engine, [secondCamera, firstCamera]);
});

function initFirstScene(engine: Engine): Camera {
const scene = engine.sceneManager.scenes[0];
const rootEntity = scene.createRootEntity();

// Create full screen camera
const cameraEntity = rootEntity.createChild("fullscreen-camera");
const camera = cameraEntity.addComponent(Camera);
cameraEntity.transform.setPosition(0, 0, 20);

// Create cube
const cubeEntity = rootEntity.createChild("cube");
cubeEntity.transform.setPosition(-2, 0, 3);
const renderer = cubeEntity.addComponent(MeshRenderer);
renderer.mesh = PrimitiveMesh.createCuboid(engine, 2, 2, 2);
const material = new BlinnPhongMaterial(engine);
material.baseColor = new Color(1, 0, 0, 1);
renderer.setMaterial(material);

return camera;
}

function initSecondScene(engine: Engine): Camera {
// Init window camera
const scene = new Scene(engine);
engine.sceneManager.addScene(scene);
const rootEntity = scene.createRootEntity();

const cameraEntity = rootEntity.createChild("window-camera");
const camera = cameraEntity.addComponent(Camera);
camera.enablePostProcess = true;
// camera.clearFlags = CameraClearFlags.None;

// @ts-ignore
const bloomEffect = scene._postProcessManager._bloomEffect;
// @ts-ignore
const tonemappingEffect = scene._postProcessManager._tonemappingEffect;
Comment on lines +63 to +66
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Avoid direct access to internal post-process manager properties.

Using @ts-ignore to access internal properties is risky and could break with future engine updates. Consider adding proper public APIs for accessing these effects if they're needed for testing.

Consider one of these approaches:

  1. Add public getters for these effects in the PostProcessManager
  2. Create a test utility function that safely accesses these properties
  3. Use public APIs to configure post-processing if available


bloomEffect.enabled = true;
tonemappingEffect.enabled = true;
bloomEffect.threshold = 0.1;
bloomEffect.intensity = 2;
Comment on lines +68 to +71
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add error handling for post-processing effects.

The code assumes the effects exist and are enabled successfully. Consider adding checks to validate the effects are available and properly initialized.

+if (!bloomEffect || !tonemappingEffect) {
+  throw new Error("Required post-processing effects are not available");
+}
 bloomEffect.enabled = true;
 tonemappingEffect.enabled = true;
 bloomEffect.threshold = 0.1;
 bloomEffect.intensity = 2;
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
bloomEffect.enabled = true;
tonemappingEffect.enabled = true;
bloomEffect.threshold = 0.1;
bloomEffect.intensity = 2;
if (!bloomEffect || !tonemappingEffect) {
throw new Error("Required post-processing effects are not available");
}
bloomEffect.enabled = true;
tonemappingEffect.enabled = true;
bloomEffect.threshold = 0.1;
bloomEffect.intensity = 2;

cameraEntity.transform.setPosition(0, 0, 20);

// Create cube
const cubeEntity = rootEntity.createChild("cube");
cubeEntity.transform.setPosition(2, 0, 3);
const renderer = cubeEntity.addComponent(MeshRenderer);
renderer.mesh = PrimitiveMesh.createCuboid(engine, 2, 2, 2);
const material = new BlinnPhongMaterial(engine);
material.baseColor = new Color(1, 0, 0, 1);
material.emissiveColor.set(1, 0, 0, 1);
renderer.setMaterial(material);

return camera;
}
85 changes: 85 additions & 0 deletions e2e/case/multi-scene-no-clear.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,85 @@
/**
* @title Multi scene no clear
* @category Advance
*/
import {
BlinnPhongMaterial,
Camera,
CameraClearFlags,
Color,
Engine,
Logger,
MeshRenderer,
PrimitiveMesh,
Scene,
WebGLEngine
} from "@galacean/engine";
import { initScreenshot, updateForE2E } from "./.mockForE2E";

Logger.enable();
WebGLEngine.create({ canvas: "canvas" }).then((engine) => {
engine.canvas.resizeByClientSize();

const firstCamera = initFirstScene(engine);
const secondCamera = initSecondScene(engine);

updateForE2E(engine);
initScreenshot(engine, [firstCamera, secondCamera]);
// initScreenshot(engine, [secondCamera, firstCamera]);
});

function initFirstScene(engine: Engine): Camera {
const scene = engine.sceneManager.scenes[0];
const rootEntity = scene.createRootEntity();

// Create full screen camera
const cameraEntity = rootEntity.createChild("fullscreen-camera");
const camera = cameraEntity.addComponent(Camera);
cameraEntity.transform.setPosition(0, 0, 20);

// Create cube
const cubeEntity = rootEntity.createChild("cube");
cubeEntity.transform.setPosition(-2, 0, 3);
const renderer = cubeEntity.addComponent(MeshRenderer);
renderer.mesh = PrimitiveMesh.createCuboid(engine, 2, 2, 2);
const material = new BlinnPhongMaterial(engine);
material.baseColor = new Color(1, 0, 0, 1);
renderer.setMaterial(material);

return camera;
}

function initSecondScene(engine: Engine): Camera {
// Init window camera
const scene = new Scene(engine);
engine.sceneManager.addScene(scene);
const rootEntity = scene.createRootEntity();

const cameraEntity = rootEntity.createChild("window-camera");
const camera = cameraEntity.addComponent(Camera);
camera.enablePostProcess = true;
camera.clearFlags = CameraClearFlags.None;

// @ts-ignore
const bloomEffect = scene._postProcessManager._bloomEffect;
// @ts-ignore
const tonemappingEffect = scene._postProcessManager._tonemappingEffect;
Comment on lines +69 to +72
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Avoid accessing internal properties with @ts-ignore.

Direct access to internal properties (_postProcessManager, _bloomEffect, _tonemappingEffect) with @ts-ignore is fragile and could break with engine updates.

Consider:

  1. Using public APIs if available
  2. If not available, propose adding public methods to access these effects
  3. Document why internal access is necessary if it can't be avoided


bloomEffect.enabled = true;
tonemappingEffect.enabled = true;
bloomEffect.threshold = 0.1;
bloomEffect.intensity = 2;
cameraEntity.transform.setPosition(0, 0, 20);

// Create cube
const cubeEntity = rootEntity.createChild("cube");
cubeEntity.transform.setPosition(2, 0, 3);
const renderer = cubeEntity.addComponent(MeshRenderer);
renderer.mesh = PrimitiveMesh.createCuboid(engine, 2, 2, 2);
const material = new BlinnPhongMaterial(engine);
material.baseColor = new Color(1, 0, 0, 1);
material.emissiveColor.set(1, 0, 0, 1);
renderer.setMaterial(material);

return camera;
}
15 changes: 15 additions & 0 deletions e2e/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -230,6 +230,21 @@ export const E2E_CONFIG = {
category: "Advance",
caseFileName: "project-loader",
threshold: 0.4
},
MultiSceneClear: {
category: "Advance",
caseFileName: "multi-scene-clear",
threshold: 0.2
},
MultiSceneNoClear: {
category: "Advance",
caseFileName: "multi-scene-no-clear",
threshold: 0.2
},
MultiCameraNoClear: {
category: "Advance",
caseFileName: "multi-camera-no-clear",
threshold: 0.2
}
}
};
3 changes: 3 additions & 0 deletions e2e/fixtures/originImage/Advance_multi-camera-no-clear.jpg
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
3 changes: 3 additions & 0 deletions e2e/fixtures/originImage/Advance_multi-scene-clear.jpg
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
3 changes: 3 additions & 0 deletions e2e/fixtures/originImage/Advance_multi-scene-no-clear.jpg
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
12 changes: 11 additions & 1 deletion packages/core/src/RenderPipeline/BasicRenderPipeline.ts
Original file line number Diff line number Diff line change
Expand Up @@ -164,7 +164,16 @@ export class BasicRenderPipeline {
rhi.activeRenderTarget(colorTarget, colorViewport, context.flipProjection, mipLevel, cubeFace);
const clearFlags = camera.clearFlags & ~(ignoreClear ?? CameraClearFlags.None);
const color = background.solidColor;
if (clearFlags !== CameraClearFlags.None) {

if (internalColorTarget) {
if (clearFlags === CameraClearFlags.All) {
rhi.clearRenderTarget(camera.engine, CameraClearFlags.All, color);
} else {
clearFlags !== CameraClearFlags.None && rhi.clearRenderTarget(camera.engine, clearFlags, color);
rhi.blitFrameBufferToInternalRT(camera.renderTarget, internalColorTarget, clearFlags, camera.viewport);
rhi.activeRenderTarget(colorTarget, colorViewport, context.flipProjection, mipLevel, cubeFace);
}
} else if (clearFlags !== CameraClearFlags.None) {
rhi.clearRenderTarget(camera.engine, clearFlags, color);
}

Expand Down Expand Up @@ -201,6 +210,7 @@ export class BasicRenderPipeline {
postProcessManager._render(context, internalColorTarget, cameraRenderTarget);
} else if (internalColorTarget) {
internalColorTarget._blitRenderTarget();

PipelineUtils.blitTexture(
engine,
<Texture2D>internalColorTarget.getColorTexture(0),
Expand Down
Loading
Loading