Enable delay initialization of the MeshoptDecoder #2146
Conversation
|
@coderabbitai review |
Actions performedReview triggered.
|
WalkthroughThese updates in the Changes
Sequence Diagram(s)Legacy Implementation FlowsequenceDiagram
participant GLTFLoader
participant MeshoptDecoder
GLTFLoader ->> MeshoptDecoder: import and usage during initialization
GLTFLoader ->> MeshoptDecoder: invoke decodeGltfBuffer()
GLTFLoader -->> MeshoptDecoder: release resources
Refactored Implementation FlowsequenceDiagram
participant GLTFLoader
participant meshoptDecoderFactory
GLTFLoader ->> meshoptDecoderFactory: invoke getMeshoptDecoder()
meshoptDecoderFactory -->> GLTFLoader: returns meshoptDecoder
GLTFLoader ->> meshoptDecoder: invoke decodeGltfBuffer()
GLTFLoader ->> meshoptDecoderFactory: async release via promise
Poem
Tip Early access features: enabledWe are currently testing the following features in early access:
Note:
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- e2e/case/gltf-meshopt.ts (2 hunks)
- packages/loader/src/GLTFLoader.ts (2 hunks)
- packages/loader/src/gltf/extensions/EXT_meshopt_compression.ts (1 hunks)
- packages/loader/src/gltf/extensions/MeshoptDecoder.ts (4 hunks)
Additional comments not posted (7)
packages/loader/src/gltf/extensions/EXT_meshopt_compression.ts (2)
5-5: Refactor import to usegetMeshoptDecoder.This change aligns with the new asynchronous pattern for initializing the MeshoptDecoder, ensuring that it's loaded only when needed.
11-18: Ensure proper handling of promises and errors increateAndParse.The method now uses promises to handle the decoder asynchronously. It's crucial to ensure that any potential rejections are handled appropriately to avoid uncaught promise rejections.
e2e/case/gltf-meshopt.ts (2)
6-6: Update imports to includeGLTFLoader.The addition of
GLTFLoaderin the imports is crucial for the test to utilize the loader functionality directly in the end-to-end testing environment.
42-42: EnsureGLTFLoader.release()is called correctly.This line ensures that resources are cleaned up after the test, which is critical for preventing memory leaks in long-running applications or extensive test suites.
packages/loader/src/GLTFLoader.ts (2)
13-13: Update import to usegetMeshoptDecoder.Changing the import to
getMeshoptDecoderis consistent with the new asynchronous initialization pattern of the MeshoptDecoder.
30-33: Refactorinitializemethod to use promises with configuration settings.This method now properly initializes the MeshoptDecoder with the configured worker count and worker usage, ensuring that the loader is fully configured before use.
packages/loader/src/gltf/extensions/MeshoptDecoder.ts (1)
Line range hint
20-215: Major refactor ofMeshoptDecoderto use promises and manage worker processes.This comprehensive refactor introduces a promise-based system for handling decoder initialization and operations, which aligns with modern asynchronous programming practices. Ensure thorough testing to validate the correct functioning of all new methods, especially in edge cases and error scenarios.
GuoLei1990
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- packages/loader/src/GLTFLoader.ts (2 hunks)
- packages/loader/src/gltf/extensions/EXT_meshopt_compression.ts (2 hunks)
- packages/loader/src/gltf/extensions/MeshoptDecoder.ts (4 hunks)
Files skipped from review due to trivial changes (1)
- packages/loader/src/GLTFLoader.ts
Additional context used
GitHub Check: lint
packages/loader/src/gltf/extensions/EXT_meshopt_compression.ts
[failure] 30-30:
Delete·
[failure] 31-31:
Delete·
[failure] 33-33:
Insert·
Additional comments not posted (7)
packages/loader/src/gltf/extensions/EXT_meshopt_compression.ts (2)
5-5: LGTM! Import change aligns with the objective.The import statement now uses
getMeshoptDecoderfor delayed initialization.
11-18: LGTM! Proper handling of asynchronous initialization.The
createAndParsemethod now correctly initializes the decoder before usage.packages/loader/src/gltf/extensions/MeshoptDecoder.ts (5)
13-18: LGTM! Promise-based initialization ensures readiness.The
readypromise ensures that the decoder is initialized before usage.
20-22: LGTM! Proper handling of decoder initialization.The
getMeshoptDecoderfunction correctly handles the initialization and returns the decoder instance.
39-43: LGTM! WebAssembly instantiation with proper promise handling.The
readypromise ensures that the WebAssembly instance is created and initialized before resolving.
44-67: LGTM! Encapsulation of decoder functionality.The resolved object includes methods for using workers, decoding GLTF buffers, and releasing resources, effectively encapsulating the decoder functionality.
216-217: LGTM! Optimized worker usage for decoding.The
decodeWorkerfunction ensures that the worker with the least pending requests is used for decoding, optimizing workload distribution.
| /** | ||
| * Worker count for transcoder, default is 4. | ||
| * @defaultValue 4 | ||
| */ |
There was a problem hiding this comment.
Fix formatting issues.
Address the formatting issues as suggested by the static analysis tool.
- /**
- * Worker count for transcoder, default is 4.
- * @defaultValue 4
- */
+ /**
+ * Worker count for transcoder, default is 4.
+ * @defaultValue 4
+ */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.
| /** | |
| * Worker count for transcoder, default is 4. | |
| * @defaultValue 4 | |
| */ | |
| /** | |
| * Worker count for transcoder, default is 4. | |
| * @defaultValue 4 | |
| */ |
Tools
GitHub Check: lint
[failure] 30-30:
Delete·
[failure] 31-31:
Delete·
[failure] 33-33:
Insert·
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- packages/loader/src/GLTFLoader.ts (2 hunks)
- packages/loader/src/gltf/extensions/MeshoptDecoder.ts (4 hunks)
Files skipped from review due to trivial changes (1)
- packages/loader/src/GLTFLoader.ts
Files skipped from review as they are similar to previous changes (1)
- packages/loader/src/gltf/extensions/MeshoptDecoder.ts
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- packages/loader/src/GLTFLoader.ts (2 hunks)
- packages/loader/src/gltf/extensions/EXT_meshopt_compression.ts (1 hunks)
- packages/loader/src/gltf/extensions/MeshoptDecoder.ts (4 hunks)
Files skipped from review due to trivial changes (1)
- packages/loader/src/GLTFLoader.ts
Files skipped from review as they are similar to previous changes (1)
- packages/loader/src/gltf/extensions/EXT_meshopt_compression.ts
Additional comments not posted (7)
packages/loader/src/gltf/extensions/MeshoptDecoder.ts (7)
13-19: Initialization of thereadypromise looks good.The
readypromise is correctly set up to handle delayed initialization.
20-22: Refactoring ofgetMeshoptDecoderfunction looks good.The function correctly checks for the
readypromise and handles theensureparameter.
39-43: Initialization of WebAssembly instance looks good.The WebAssembly instance is correctly initialized within the
readypromise.
44-67: Addition of methods to the resolved object looks good.The
workerCount,useWorkers,decodeGltfBuffer, andreleasemethods are correctly defined and added to the resolved object.
143-158: Definition ofdecodefunction looks good.The
decodefunction is correctly defined within the worker initialization code.
160-175: Definition ofworkerProcessfunction looks good.The
workerProcessfunction correctly handles messages from the main thread.
216-217: Return ofreadypromise looks good.The
readypromise is correctly returned from thegetMeshoptDecoderfunction.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- packages/loader/src/GLTFLoader.ts (2 hunks)
- packages/loader/src/gltf/extensions/MeshoptDecoder.ts (4 hunks)
Files skipped from review as they are similar to previous changes (2)
- packages/loader/src/GLTFLoader.ts
- packages/loader/src/gltf/extensions/MeshoptDecoder.ts
| @@ -106,39 +139,39 @@ const MeshoptDecoder = (function () { | |||
| ".then(function(result) {instance = result.instance; instance.exports.__wasm_call_ctors();});\n" + | |||
There was a problem hiding this comment.
function initWorkers(count) {
const script = generateWorkerScript();
const blob = new Blob([script], { type: "text/javascript" });
const url = URL.createObjectURL(blob);
for (let i = 0; i < count; ++i) {
workers[i] = createWorker(url);
}
URL.revokeObjectURL(url);
}
function generateWorkerScript() {
return `
var instance;
var ready = WebAssembly.instantiate(new Uint8Array([${new Uint8Array(unpack(wasm)).toString()}]), {})
.then(result => {
instance = result.instance;
instance.exports.__wasm_call_ctors();
});
self.onmessage = async function(event) {
await ready;
const data = event.data;
try {
const target = new Uint8Array(data.count * data.size);
decode(instance.exports[data.mode], target, data.count, data.size, data.source, instance.exports[data.filter]);
self.postMessage({ id: data.id, count: data.count, action: "resolve", value: target }, [target.buffer]);
} catch (error) {
self.postMessage({ id: data.id, count: data.count, action: "reject", value: error });
}
};
`;
}
Please check if the PR fulfills these requirements
Summary by CodeRabbit
MeshoptDecoderis handled. This change ensures more efficient initialization and usage of the decoder in theGLTFLoader.