Skip to content
Draft
Show file tree
Hide file tree
Changes from all 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
13 changes: 2 additions & 11 deletions packages/effects-core/src/asset-manager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,6 @@ export class AssetManager implements Disposable {
const startTime = performance.now();
const timeInfoMessages: string[] = [];
const gpuInstance = renderer?.engine.gpuCapability;
const asyncShaderCompile = gpuInstance?.detail?.asyncShaderCompile ?? false;
const compressedTexture = gpuInstance?.detail.compressedTexture ?? COMPRESSED_TEXTURE.NONE;
const timeInfos: Record<string, number> = {};
let loadTimer: number;
Expand Down Expand Up @@ -173,7 +172,8 @@ export class AssetManager implements Disposable {
const [loadedBins, loadedImages] = await Promise.all([
hookTimeInfo('processBins', () => this.processBins(bins)),
hookTimeInfo('processImages', () => this.processImages(images, compressedTexture)),
hookTimeInfo(`${asyncShaderCompile ? 'async' : 'sync'}Compile`, () => this.precompile(compositions, pluginSystem, renderer, options)),
hookTimeInfo('precompile', () => this.precompile(compositions, pluginSystem, renderer, options)),
hookTimeInfo('processFontURL', () => this.processFontURL(fonts as spec.FontDefine[])),
]);

if (renderer) {
Expand All @@ -186,7 +186,6 @@ export class AssetManager implements Disposable {
}
}

await hookTimeInfo('processFontURL', () => this.processFontURL(fonts as spec.FontDefine[]));
const loadedTextures = await hookTimeInfo('processTextures', () => this.processTextures(loadedImages, loadedBins, jsonScene, renderer!.engine));

scene = {
Expand Down Expand Up @@ -231,15 +230,7 @@ export class AssetManager implements Disposable {
if (!renderer || !renderer.getShaderLibrary()) {
return;
}
const shaderLibrary = renderer?.getShaderLibrary();

await pluginSystem?.precompile(compositions, renderer, options);

await new Promise(resolve => {
shaderLibrary?.compileAllShaders(() => {
resolve(null);
});
});
}

private async processJSON (json: JSONValue) {
Expand Down
17 changes: 14 additions & 3 deletions packages/effects-core/src/composition.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,10 +23,16 @@ import { EventEmitter } from './events';
import type { PostProcessVolume } from './components/post-process-volume';

export interface CompositionStatistic {
loadTime: number,
loadStart: number,
loadTime: number,
/**
* Shader 编译耗时
*/
compileTime: number,
/**
* 从加载到渲染第一帧的时间(含 Shader 编译)
*/
firstFrameTime: number,
precompileTime: number,
}

export interface MessageItem {
Expand Down Expand Up @@ -247,7 +253,12 @@ export class Composition extends EventEmitter<CompositionEvent<Composition>> imp
this.renderer = renderer;
this.texInfo = imageUsage ?? {};
this.event = event;
this.statistic = { loadTime: totalTime ?? 0, loadStart: scene.startTime ?? 0, firstFrameTime: 0, precompileTime: scene.timeInfos['asyncCompile'] ?? scene.timeInfos['syncCompile'] };
this.statistic = {
loadStart: scene.startTime ?? 0,
loadTime: totalTime ?? 0,
compileTime: 0,
firstFrameTime: 0,
};
this.reusable = reusable;
this.speed = speed;
this.autoRefTex = !this.keepResource && imageUsage && this.rootItem.endBehavior !== spec.EndBehavior.restart;
Expand Down
7 changes: 6 additions & 1 deletion packages/effects/src/player.ts
Original file line number Diff line number Diff line change
Expand Up @@ -384,6 +384,8 @@ export class Player extends EventEmitter<PlayerEvent<Player>> implements Disposa
}
}

const compileStart = performance.now();

await new Promise(resolve => {
this.renderer.getShaderLibrary()?.compileAllShaders(() => {
resolve(null);
Expand All @@ -397,10 +399,13 @@ export class Player extends EventEmitter<PlayerEvent<Player>> implements Disposa
composition.pause();
}

const firstFrameTime = performance.now() - last + composition.statistic.loadTime;
const firstFrameTime = performance.now() - last;
const compileTime = performance.now() - compileStart;

composition.statistic.firstFrameTime = firstFrameTime;
composition.statistic.compileTime = compileTime;
logger.info(`First frame: [${composition.name}]${firstFrameTime.toFixed(4)}ms.`);
logger.info(`Shader compile: [${composition.name}]${compileTime.toFixed(4)}ms.`);

return composition;
}
Expand Down
41 changes: 41 additions & 0 deletions web-packages/demo/html/shader-compile.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
<!DOCTYPE html>
<html lang="en">
<head>
<meta charset="UTF-8">
<meta name="viewport" content="width=device-width, initial-scale=1.0">
<title>Shader 编译测试 - demo</title>
<style>
html, body, div, canvas { margin: 0; padding: 0; }
.container {
visibility: hidden;
opacity: 0;
width: 188px;
height: 334px;
position: fixed;
bottom: 0%;
left: 50%;
transform: translate(-50%, 0) scale(0);
transition: transform 0.5s ease-in-out;
background-color: rgba(0,0,0,255);
}
.container.active {
visibility: visible;
z-index: -1;
opacity: 1;
transform: translate(-50%, 0) scale(1);
}
ul {
background-color: rgba(255,255,255,0.5);
}
</style>
Comment on lines +7 to +30
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider enhancing CSS for better maintainability and user experience.

While the current CSS is functional, there are opportunities for improvement:

  1. The container's background is set to solid black (rgba(0,0,0,255)), which might affect content visibility.
  2. The z-index of -1 on the active container might cause layering issues.
  3. The styles could benefit from using CSS variables for better maintainability.

Consider the following improvements:

 <style>
   html, body, div, canvas { margin: 0; padding: 0; }
+  :root {
+    --container-width: 188px;
+    --container-height: 334px;
+    --container-bg: rgba(0, 0, 0, 0.8);
+    --transition-duration: 0.5s;
+  }
   .container {
     visibility: hidden;
     opacity: 0;
-    width: 188px;
-    height: 334px;
+    width: var(--container-width);
+    height: var(--container-height);
     position: fixed;
     bottom: 0%;
     left: 50%;
     transform: translate(-50%, 0) scale(0);
-    transition: transform 0.5s ease-in-out;
-    background-color: rgba(0,0,0,255);
+    transition: transform var(--transition-duration) ease-in-out, opacity var(--transition-duration) ease-in-out;
+    background-color: var(--container-bg);
   }
   .container.active {
     visibility: visible;
-    z-index: -1;
+    z-index: 1;
     opacity: 1;
     transform: translate(-50%, 0) scale(1);
   }
   ul {
-    background-color: rgba(255,255,255,0.5);
+    background-color: rgba(255, 255, 255, 0.8);
   }
 </style>

These changes improve readability, maintainability, and potentially the visual appearance of the interface.

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
<style>
html, body, div, canvas { margin: 0; padding: 0; }
.container {
visibility: hidden;
opacity: 0;
width: 188px;
height: 334px;
position: fixed;
bottom: 0%;
left: 50%;
transform: translate(-50%, 0) scale(0);
transition: transform 0.5s ease-in-out;
background-color: rgba(0,0,0,255);
}
.container.active {
visibility: visible;
z-index: -1;
opacity: 1;
transform: translate(-50%, 0) scale(1);
}
ul {
background-color: rgba(255,255,255,0.5);
}
</style>
<style>
html, body, div, canvas { margin: 0; padding: 0; }
:root {
--container-width: 188px;
--container-height: 334px;
--container-bg: rgba(0, 0, 0, 0.8);
--transition-duration: 0.5s;
}
.container {
visibility: hidden;
opacity: 0;
width: var(--container-width);
height: var(--container-height);
position: fixed;
bottom: 0%;
left: 50%;
transform: translate(-50%, 0) scale(0);
transition: transform var(--transition-duration) ease-in-out, opacity var(--transition-duration) ease-in-out;
background-color: var(--container-bg);
}
.container.active {
visibility: visible;
z-index: 1;
opacity: 1;
transform: translate(-50%, 0) scale(1);
}
ul {
background-color: rgba(255, 255, 255, 0.8);
}
</style>

</head>
<body>
<dl id="J-statistic"></dl>
<div>
<button id="J-button" style="height: 32px; width: 100px;">播放</button>
</div>
<div class="container" id="J-container"></div>

<script type="module" src="../src/shader-compile.ts"></script>
</body>
</html>
1 change: 1 addition & 0 deletions web-packages/demo/index.html
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
<div class="am-list-body">
<a class="am-list-item" href="html/single.html">单项目测试</a>
<a class="am-list-item" href="html/post-processing.html">后处理测试</a>
<a class="am-list-item" href="html/shader-compile.html">Shader 编译测试</a>
<a class="am-list-item" href="html/dashboard.html">Dashboard</a>
<a class="am-list-item" href="html/interactive.html">Interactive</a>
<a class="am-list-item" href="html/compressed.html">压缩纹理</a>
Expand Down
52 changes: 52 additions & 0 deletions web-packages/demo/src/shader-compile.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
import type { Composition } from '@galacean/effects';
import { Player } from '@galacean/effects';
import '@galacean/effects-plugin-spine';

// 大量粒子
// const json = 'https://mdn.alipayobjects.com/mars/afts/file/A*aCeuQ5RQZj4AAAAAAAAAAAAADlB4AQ';
// 新年烟花
const json = [
'https://gw.alipayobjects.com/os/gltf-asset/mars-cli/ILDKKFUFMVJA/1705406034-80896.json',
'https://mdn.alipayobjects.com/graph_jupiter/afts/file/A*qTquTKYbk6EAAAAAAAAAAAAADsF2AQ',
];
// 混合测试
// const json = [
// 'https://mdn.alipayobjects.com/mars/afts/file/A*QyX8Rp-4fmUAAAAAAAAAAAAADlB4AQ',
// 'https://mdn.alipayobjects.com/mars/afts/file/A*bi3HRobVsk8AAAAAAAAAAAAADlB4AQ',
// 'https://mdn.alipayobjects.com/graph_jupiter/afts/file/A*sEdkT5cdXGEAAAAAAAAAAAAADsF2AQ',
// ];
// 塔奇
// const json = 'https://mdn.alipayobjects.com/mars/afts/file/A*uU2JRIjcLIcAAAAAAAAAAAAADlB4AQ';
// const json = 'https://gw.alipayobjects.com/os/gltf-asset/mars-cli/TAJIINQOUUKP/-799304223-0ee5d.json';
const container = document.getElementById('J-container');

document.getElementById('J-button')!.addEventListener('click', () => {
Comment on lines +21 to +23
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Improve error handling for DOM element selection

Consider adding checks to ensure DOM elements exist before using them:

const container = document.getElementById('J-container');
const button = document.getElementById('J-button');

if (!container || !button) {
  console.error('Required DOM elements not found');
  // Handle the error appropriately
  return;
}

button.addEventListener('click', () => {
  // ... existing code ...
});

This approach prevents potential runtime errors and provides better error feedback.

(async () => {
try {
container?.classList.add('active');

const player = new Player({
container,
// renderFramework: 'webgl2',
});
const compositions = await player.loadScene(Array.isArray(json) ? json : [json]) as unknown as Composition[];
Comment on lines +28 to +32
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Refine type handling and error management

  1. The type assertion as unknown as Composition[] suggests a potential type mismatch. Consider updating the loadScene method's return type or use a type guard instead of assertion:

    const compositions = await player.loadScene(Array.isArray(json) ? json : [json]);
    if (!Array.isArray(compositions) || !compositions.every(c => c instanceof Composition)) {
      throw new Error('Invalid composition data received');
    }
  2. Implement more specific error handling to provide better feedback:

    try {
      // ... existing code ...
    } catch (e) {
      if (e instanceof Error) {
        console.error(`Failed to load scene: ${e.message}`);
      } else {
        console.error('An unknown error occurred while loading the scene');
      }
      // Consider updating UI to reflect the error state
    }


compositions.forEach(composition => {
const dt = document.createElement('dt');

dt.innerHTML = `>>> composition: ${composition.name}`;
document.getElementById('J-statistic')?.appendChild(dt);

for (const key in composition.statistic) {
const p = document.createElement('dd');

// @ts-expect-error
p.innerHTML = `${key}: ${composition.statistic[key]}`;
document.getElementById('J-statistic')?.appendChild(p);
}
});
Comment on lines +34 to +47
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

⚠️ Potential issue

Enhance performance, security, and error handling in statistics display

  1. For better performance with large datasets, consider using DocumentFragment:

    const fragment = document.createDocumentFragment();
    compositions.forEach(composition => {
      // ... create elements and append to fragment
    });
    document.getElementById('J-statistic')?.appendChild(fragment);
  2. Add a check for the existence of the statistics container:

    const statisticContainer = document.getElementById('J-statistic');
    if (!statisticContainer) {
      console.error('Statistics container not found');
      return;
    }
  3. To prevent potential XSS vulnerabilities, use textContent instead of innerHTML:

    dt.textContent = `>>> composition: ${composition.name}`;
    // ...
    p.textContent = `${key}: ${composition.statistic[key]}`;
  4. Consider creating a dedicated function for creating statistic elements to improve readability and maintainability.

} catch (e) {
console.error('biz', e);
}
})();
});
Comment on lines +23 to +52
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Improve overall structure and error handling

  1. Consider extracting the main functionality into a separate async function for better readability and testability:

    async function loadAndDisplayCompositions() {
      try {
        // ... existing code ...
      } catch (e) {
        handleError(e);
      }
    }
    
    function handleError(e: unknown) {
      console.error('Failed to load or display compositions:', e);
      // Update UI to show error message to the user
      const errorMessage = e instanceof Error ? e.message : 'An unknown error occurred';
      document.getElementById('J-error-message')?.textContent = errorMessage;
    }
    
    document.getElementById('J-button')?.addEventListener('click', loadAndDisplayCompositions);
  2. Add appropriate loading and error states to improve user experience:

    function showLoading(isLoading: boolean) {
      const loadingElement = document.getElementById('J-loading');
      if (loadingElement) {
        loadingElement.style.display = isLoading ? 'block' : 'none';
      }
    }
    
    async function loadAndDisplayCompositions() {
      showLoading(true);
      try {
        // ... existing code ...
      } catch (e) {
        handleError(e);
      } finally {
        showLoading(false);
      }
    }

These changes will improve the code's structure, error handling, and user experience.

1 change: 1 addition & 0 deletions web-packages/demo/vite.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ export default defineConfig(({ mode }) => {
'dynamic-image': resolve(__dirname, 'html/dynamic-image.html'),
'dynamic-video': resolve(__dirname, 'html/dynamic-video.html'),
'render-level': resolve(__dirname, 'html/render-level.html'),
'shader-compile': resolve(__dirname, 'html/shader-compile.html'),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

💡 Codebase verification

Compilation time tracking not implemented

The shader-compile.html file does not include any implementation for compilation time tracking. Please add the necessary code to enable tracking of compilation time as per the PR objectives.

Analysis chain

Reminder: Implement compilation time tracking

While this configuration change correctly includes the new shader-compile.html file, ensure that the actual implementation of compilation time tracking is present in the HTML file or its associated JavaScript. This is crucial to meet the PR objective of adding compilation time statistics.

To verify the implementation of compilation time tracking, let's check the contents of the shader-compile.html file:



Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for compilation time tracking implementation in shader-compile.html

# Search for keywords related to time tracking in the shader-compile.html file
rg -i "time|duration|performance|measure" "html/shader-compile.html"

Length of output: 228


Script:

#!/bin/bash
# Description: Locate the shader-compile.html file in the repository

fd "shader-compile\.html$" .

Length of output: 75


Script:

#!/bin/bash
# Description: Verify compilation time tracking implementation in shader-compile.html

rg -i "time|duration|performance|measure" "./web-packages/demo/html/shader-compile.html"

Length of output: 88

'local-file': resolve(__dirname, 'html/local-file.html'),
'post-processing': resolve(__dirname, 'html/post-processing.html'),
'single': resolve(__dirname, 'html/single.html'),
Expand Down