Support background audio #2449
Support background audio #2449
Conversation
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (2)
packages/core/src/audio/AudioSource.ts (2)
7-37: Enhance class and property documentationWhile the basic documentation is present, consider adding:
- Parameter descriptions for properties
- Units for numeric properties (e.g., volume range, playback rate range)
- Examples of typical usage
Example documentation improvement:
/** * Audio Source Component. + * + * Manages audio playback for an entity, including volume control, + * playback rate, looping, and pause/resume functionality. + * + * @example + * ```ts + * const audioSource = entity.addComponent(AudioSource); + * audioSource.clip = audioClip; + * audioSource.volume = 0.5; // 50% volume + * audioSource.play(); + * ``` */
237-240: Simplify boolean expression in _canPlayThe boolean conversion is unnecessary and can be simplified.
Apply this fix:
private _canPlay(): boolean { - const isValidClip = this._clip?._getAudioSource() ? true : false; + const isValidClip = !!this._clip?._getAudioSource(); return isValidClip && AudioManager.isAudioContextRunning(); }🧰 Tools
🪛 Biome (1.9.4)
[error] 238-238: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with(lint/complexity/noUselessTernary)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/core/src/audio/AudioSource.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
packages/core/src/audio/AudioSource.ts
[error] 48-48: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 49-49: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 238-238: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with
(lint/complexity/noUselessTernary)
🔇 Additional comments (1)
packages/core/src/audio/AudioSource.ts (1)
215-228:
Add error handling for audio node initialization and playback
The _initSourceNode method should handle potential errors during audio node setup and playback.
Apply this fix:
private _initSourceNode(startTime: number): void {
- const context = AudioManager.getContext();
- const sourceNode = context.createBufferSource();
-
- sourceNode.buffer = this._clip._getAudioSource();
- sourceNode.playbackRate.value = this._playbackRate;
- sourceNode.loop = this._loop;
- sourceNode.>
- this._sourceNode = sourceNode;
-
- sourceNode.connect(this._gainNode);
-
- this._sourceNode.start(0, startTime);
+ try {
+ const context = AudioManager.getContext();
+ const sourceNode = context.createBufferSource();
+ const audioBuffer = this._clip._getAudioSource();
+
+ if (!audioBuffer) {
+ throw new Error("Invalid audio buffer");
+ }
+
+ sourceNode.buffer = audioBuffer;
+ sourceNode.playbackRate.value = this._playbackRate;
+ sourceNode.loop = this._loop;
+ sourceNode.>
+ this._sourceNode = sourceNode;
+
+ sourceNode.connect(this._gainNode);
+ sourceNode.start(0, startTime);
+ } catch (error) {
+ this.engine?.logger?.error("Failed to initialize audio source:", error);
+ this._sourceNode = null;
+ throw error;
+ }
}Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
packages/core/src/audio/AudioSource.ts (2)
11-12: Enhance documentation for playOnEnabled propertyThe current documentation is basic. Consider adding more details about:
- Default value
- Behavior when disabled
- Interaction with pause/stop states
- /** If set to true, the audio component automatically begins to play on startup. */ + /** + * Controls automatic playback behavior on component startup. + * @default true + * @remarks + * - When true, audio will automatically play when the component is enabled + * - When false, manual play() call is required + * - Has no effect if the component is already playing + */
237-240: Simplify boolean expression in _canPlay methodThe boolean conversion is unnecessary and can be simplified.
private _canPlay(): boolean { - const isValidClip = this._clip?._getAudioSource() ? true : false; - return isValidClip && AudioManager.isAudioContextRunning(); + return !!this._clip?._getAudioSource() && AudioManager.isAudioContextRunning(); }🧰 Tools
🪛 Biome (1.9.4)
[error] 238-238: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with(lint/complexity/noUselessTernary)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/core/src/audio/AudioSource.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
packages/core/src/audio/AudioSource.ts
[error] 48-48: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 49-49: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 238-238: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with
(lint/complexity/noUselessTernary)
🔇 Additional comments (4)
packages/core/src/audio/AudioSource.ts (4)
45-53:
Fix reference counting and error handling in clip setter
The current implementation has several issues that need to be addressed:
- Reference counting order could lead to memory leaks if
_addReferCountthrows - Missing error handling
- Non-idiomatic use of optional chaining
Apply this fix:
set clip(value: AudioClip) {
const lastClip = this._clip;
if (lastClip !== value) {
- lastClip && lastClip._addReferCount(-1);
- value && value._addReferCount(1);
+ try {
+ value?._addReferCount(1);
+ lastClip?._addReferCount(-1);
+ } catch (error) {
+ // Rollback on error
+ value?._addReferCount(-1);
+ throw error;
+ }
this._clip = value;
this.stop();
}
}🧰 Tools
🪛 Biome (1.9.4)
[error] 48-48: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 49-49: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
125-132:
Fix time calculation in paused state
The current time calculation in paused state is incorrect. It should return the stored pause time instead of calculating a new time difference.
get time(): number {
if (this._isPlaying) {
const currentTime = AudioManager.getContext().currentTime;
return currentTime - this._playTime;
} else {
- return this._pausedTime > 0 ? this._pausedTime - this._playTime : 0;
+ return this._pausedTime > 0 ? this._pausedTime : 0;
}
}205-209: 🛠️ Refactor suggestion
Add null check in _onDestroy method
The _onDestroy method sets clip to null without checking if it exists, which could cause unnecessary reference count operations.
protected override _onDestroy(): void {
super._onDestroy();
this.stop();
- this.clip = null;
+ if (this._clip) {
+ this.clip = null;
+ }
}Likely invalid or redundant comment.
215-228:
Add error handling in _initSourceNode method
The method should handle potential errors during source node initialization and buffer assignment.
private _initSourceNode(startTime: number): void {
const context = AudioManager.getContext();
- const sourceNode = context.createBufferSource();
-
- sourceNode.buffer = this._clip._getAudioSource();
- sourceNode.playbackRate.value = this._playbackRate;
- sourceNode.loop = this._loop;
- sourceNode.>
- this._sourceNode = sourceNode;
-
- sourceNode.connect(this._gainNode);
-
- this._sourceNode.start(0, startTime);
+ try {
+ const sourceNode = context.createBufferSource();
+ const audioBuffer = this._clip._getAudioSource();
+ if (!audioBuffer) {
+ throw new Error("Invalid audio buffer");
+ }
+ sourceNode.buffer = audioBuffer;
+ sourceNode.playbackRate.value = this._playbackRate;
+ sourceNode.loop = this._loop;
+ sourceNode.>
+ this._sourceNode = sourceNode;
+
+ sourceNode.connect(this._gainNode);
+ sourceNode.start(0, startTime);
+ } catch (error) {
+ this.engine?.logger?.error("Failed to initialize audio source:", error);
+ throw error;
+ }
}Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
packages/core/src/audio/AudioManager.ts (1)
5-53: Consider refactoring to a singleton patternThe current static-only class design could be improved for better testability and maintainability.
Consider refactoring to a singleton pattern:
-export class AudioManager { +export class AudioManager { + private static instance: AudioManager; + private context: AudioContext; + private gainNode: GainNode; + private isResuming = false; + + private constructor() { + // Private constructor to prevent direct construction + } + + public static getInstance(): AudioManager { + if (!AudioManager.instance) { + AudioManager.instance = new AudioManager(); + } + return AudioManager.instance; + } + + public getContext(): AudioContext { + if (!this.context) { + this.context = new window.AudioContext(); + this.setupContextResume(); + } + return this.context; + }🧰 Tools
🪛 Biome (1.9.4)
[error] 5-53: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
packages/core/src/audio/AudioSource.ts (1)
241-244: Simplify _canPlay logicThe boolean conversion can be simplified.
private _canPlay(): boolean { - const isValidClip = this._clip?._getAudioSource() ? true : false; + const isValidClip = !!this._clip?._getAudioSource(); return isValidClip && AudioManager.isAudioContextRunning(); }🧰 Tools
🪛 Biome (1.9.4)
[error] 242-242: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with(lint/complexity/noUselessTernary)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/core/src/audio/AudioManager.ts(1 hunks)packages/core/src/audio/AudioSource.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
packages/core/src/audio/AudioSource.ts
[error] 48-48: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 49-49: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 242-242: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with
(lint/complexity/noUselessTernary)
packages/core/src/audio/AudioManager.ts
[error] 5-53: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
🔇 Additional comments (6)
packages/core/src/audio/AudioManager.ts (2)
41-52: Add cleanup mechanism for event listeners
The event listeners should be cleaned up if the unlock attempt fails or times out to prevent memory leaks.
10-22:
Add error handling for AudioContext creation
The context creation should handle potential errors, especially for browsers that don't support Web Audio API.
Add error handling:
static getContext(): AudioContext {
let context = AudioManager._context;
if (!context) {
+ try {
AudioManager._context = context = new window.AudioContext();
+ } catch (error) {
+ console.error("Failed to create AudioContext. Web Audio API might not be supported:", error);
+ throw error;
+ }Likely invalid or redundant comment.
packages/core/src/audio/AudioSource.ts (4)
45-53: Improve clip setter safety and reference counting
The current implementation has potential memory leaks if _addReferCount throws.
🧰 Tools
🪛 Biome (1.9.4)
[error] 48-48: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 49-49: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
141-147: Add error handling for audio node initialization
The constructor should handle potential errors when creating audio nodes.
234-239: Add null checks in _clearSourceNode method
The method should check if source node exists before attempting operations.
219-232:
Add error handling and cleanup in _initSourceNode
The source node initialization needs proper error handling and cleanup.
Add error handling:
private _initSourceNode(startTime: number): void {
+ try {
const context = AudioManager.getContext();
const sourceNode = context.createBufferSource();
+ if (!this._clip?._getAudioSource()) {
+ throw new Error("Invalid audio source");
+ }
sourceNode.buffer = this._clip._getAudioSource();
sourceNode.playbackRate.value = this._playbackRate;
sourceNode.loop = this._loop;
sourceNode.
this._sourceNode = sourceNode;
sourceNode.connect(this._gainNode);
this._sourceNode.start(0, startTime);
+ } catch (error) {
+ this._sourceNode = null;
+ console.error("Failed to initialize audio source:", error);
+ throw error;
+ }
}Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
packages/core/src/audio/AudioManager.ts (1)
32-38: Enhance warning message with troubleshooting infoConsider providing more detailed guidance in the warning message to help developers understand how to resolve the issue.
static isAudioContextRunning(): boolean { if (AudioManager.getContext().state !== "running") { - console.warn("AudioContext is not running, user interaction required."); + console.warn( + "AudioContext is not running. This is likely due to browser autoplay policies. " + + "User interaction (click, tap) is required to start audio playback." + ); return false; } return true; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/core/src/audio/AudioManager.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
packages/core/src/audio/AudioManager.ts
[error] 5-52: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
🔇 Additional comments (2)
packages/core/src/audio/AudioManager.ts (2)
1-9: LGTM! Static-only class is appropriate here
While the static analyzer suggests avoiding classes with only static members, this is actually a good use case for a static class as it manages global audio context and gain node as singletons, which aligns with the Web Audio API's design patterns.
10-21:
Add error handling and event listener cleanup
The method needs improvements in error handling and resource management:
- AudioContext creation should handle browser compatibility issues
- Event listeners should be cleaned up when no longer needed
Apply this diff:
static getContext(): AudioContext {
let context = AudioManager._context;
if (!context) {
+ try {
AudioManager._context = context = new AudioContext();
+ } catch (error) {
+ console.error("Failed to create AudioContext. Web Audio API might not be supported:", error);
+ throw error;
+ }
// Safari can't resume audio context without element interaction
+ const cleanup = () => {
+ document.removeEventListener("pointerdown", AudioManager._tryResume, true);
+ document.removeEventListener("touchend", AudioManager._tryResume, true);
+ document.removeEventListener("touchstart", AudioManager._tryResume, true);
+ };
document.addEventListener("pointerdown", AudioManager._tryResume, true);
document.addEventListener("touchend", AudioManager._tryResume, true);
document.addEventListener("touchstart", AudioManager._tryResume, true);
+ // Cleanup listeners if context enters running state
+ context.addEventListener('statechange', () => {
+ if (context.state === 'running') {
+ cleanup();
+ }
+ });
}
return context;
}There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/core/src/audio/AudioManager.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
packages/core/src/audio/AudioManager.ts
[error] 5-52: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
🔇 Additional comments (4)
packages/core/src/audio/AudioManager.ts (4)
23-30: Add error handling for gain node creation
40-51: Add error handling and timeout for resume attempts
10-21: 🛠️ Refactor suggestion
Remove window reference and add cleanup mechanism
The implementation needs several improvements:
- Remove the
windowreference as previously suggested - Add cleanup mechanism for event listeners
- Add error handling (as previously mentioned)
static getContext(): AudioContext {
let context = AudioManager._context;
if (!context) {
- AudioManager._context = context = new window.AudioContext();
+ try {
+ AudioManager._context = context = new AudioContext();
+ } catch (error) {
+ console.error("Failed to create AudioContext:", error);
+ throw error;
+ }
// Safari can't resume audio context without element interaction
+ const cleanup = () => {
+ document.removeEventListener("pointerdown", AudioManager._tryResume, true);
+ document.removeEventListener("touchend", AudioManager._tryResume, true);
+ document.removeEventListener("touchstart", AudioManager._tryResume, true);
+ };
+
document.addEventListener("pointerdown", AudioManager._tryResume, true);
document.addEventListener("touchend", AudioManager._tryResume, true);
document.addEventListener("touchstart", AudioManager._tryResume, true);
+
+ // Cleanup if context fails to initialize within 5 seconds
+ setTimeout(() => {
+ if (context.state !== "running") {
+ cleanup();
+ }
+ }, 5000);
}
return context;
}1-52: Verify browser compatibility
The implementation uses Web Audio API features. Let's verify browser support and consider adding fallbacks if needed.
✅ Verification successful
Let me analyze the output and generate a final response.
Browser compatibility is well-handled
The implementation already includes necessary browser compatibility handling:
- Uses standard Web Audio API features with proper context initialization
- Includes fallback handling for Safari's audio context resuming requirement
- Implements user interaction listeners (pointerdown, touchend, touchstart) to handle autoplay restrictions
- Provides warning messages when audio context is not running
- Uses standard GainNode for volume control
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for browser compatibility checks or polyfills in the codebase
# Look for existing browser compatibility checks
rg -i "audiocontext|webaudio|audio.*api" --type ts
# Look for potential polyfills
rg -i "polyfill.*audio|audio.*polyfill" --type ts
Length of output: 56244
🧰 Tools
🪛 Biome (1.9.4)
[error] 5-52: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
* fix: env restore
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
packages/loader/src/EnvLoader.ts (2)
25-44: Consider marking_setTextureByBufferasprivateThe method
_setTextureByBufferappears to be intended for internal use. Marking it asprivateenhances encapsulation and reflects its intended access level.
75-94: Enhance error handling inEnvContentRestorerIn the
restoreContentmethod ofEnvContentRestorer, consider adding more detailed error handling or logging to aid in debugging if the request fails.packages/loader/src/Texture2DLoader.ts (1)
Line range hint
22-24: Consider using type validation instead of type assertionThe type assertion
<RequestConfig>could be unsafe. Consider adding runtime validation or using a type guard.- const requestConfig = <RequestConfig>{ + const requestConfig: RequestConfig = { ...item, type: "image" };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (14)
packages/core/src/asset/Loader.ts(0 hunks)packages/core/src/asset/request.ts(2 hunks)packages/core/src/index.ts(2 hunks)packages/loader/src/AudioLoader.ts(1 hunks)packages/loader/src/EnvLoader.ts(2 hunks)packages/loader/src/GLTFContentRestorer.ts(1 hunks)packages/loader/src/HDRLoader.ts(1 hunks)packages/loader/src/Texture2DContentRestorer.ts(1 hunks)packages/loader/src/Texture2DLoader.ts(1 hunks)packages/loader/src/TextureCubeContentRestorer.ts(1 hunks)packages/loader/src/TextureCubeLoader.ts(1 hunks)packages/loader/src/gltf/parser/GLTFBufferParser.ts(1 hunks)packages/loader/src/gltf/parser/GLTFSchemaParser.ts(1 hunks)tests/src/core/audio/AudioSource.test.ts(1 hunks)
💤 Files with no reviewable changes (1)
- packages/core/src/asset/Loader.ts
✅ Files skipped from review due to trivial changes (4)
- packages/loader/src/Texture2DContentRestorer.ts
- packages/loader/src/TextureCubeContentRestorer.ts
- packages/loader/src/gltf/parser/GLTFSchemaParser.ts
- packages/loader/src/gltf/parser/GLTFBufferParser.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/core/src/index.ts
- packages/loader/src/AudioLoader.ts
- tests/src/core/audio/AudioSource.test.ts
🧰 Additional context used
🪛 eslint
packages/core/src/asset/request.ts
[error] 21-21: Delete ·
(prettier/prettier)
🪛 GitHub Check: lint
packages/core/src/asset/request.ts
[failure] 21-21:
Delete ·
🔇 Additional comments (9)
packages/loader/src/TextureCubeLoader.ts (1)
5-6: Use of RequestConfig improves request consistency
Incorporating RequestConfig when constructing requestConfig enhances the consistency and flexibility of request handling.
packages/core/src/asset/request.ts (1)
33-36: RequestConfig addition enhances flexibility
The introduction of RequestConfig and its integration into the request function improves configurability for retries, timeouts, and response types.
packages/loader/src/GLTFContentRestorer.ts (2)
7-10: Improved request handling with RequestConfig
Updating imports and utilizing RequestConfig in request functions enhances consistency across loaders and content restorers.
Line range hint 70-72: Include RequestConfig in BufferRequestInfo
Adding RequestConfig to BufferRequestInfo ensures that request parameters are properly managed during content restoration.
packages/loader/src/HDRLoader.ts (2)
8-8: Enhancement of request handling using RequestConfig
Implementing RequestConfig in HDRLoader streamlines request configurations and aligns with the updated request management approach.
Line range hint 120-126: Update HDRContentRestorer constructor with RequestConfig
Including requestConfig in the HDRContentRestorer constructor ensures consistency in request parameters during content restoration.
packages/loader/src/Texture2DLoader.ts (3)
5-12: LGTM: Import changes align with request handling standardization
The addition of RequestConfig and resourceLoader imports supports the standardized request handling approach mentioned in the summary.
Line range hint 57-71: LGTM: Well-documented interface definition
The Texture2DParams interface is properly documented and includes all necessary texture parameters with appropriate types.
Line range hint 47-47: Verify content restoration implementation
Let's verify that the content restorer is properly implemented across the codebase.
✅ Verification successful
Let me gather more information about the specific content restorer implementations to verify their consistency.
Let me try a different approach to find the content restorer implementations.
Content restoration implementation is properly implemented
The verification shows that the content restoration system is correctly implemented:
- The base
ContentRestorerclass is properly defined in core with the required abstractrestoreContent()method Texture2DContentRestorercorrectly extends the base class and implements the restoration logic- The implementation follows the same pattern as other content restorers (TextureCube, HDR, Env, GLTF)
- The registration in
Texture2DLoaderis consistent with other loaders, usingaddContentRestorer()with proper parameters
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other content restorer implementations to ensure consistency
ast-grep --pattern 'class $_ContentRestorer {
$$$
restore() {
$$$
}
$$$
}'
# Check for content restorer usage patterns
rg "addContentRestorer" -A 2
Length of output: 5503
Script:
#!/bin/bash
# Check Texture2DContentRestorer implementation
ast-grep --pattern 'class Texture2DContentRestorer {
$$$
restore() {
$$$
}
}'
# Check other texture-related content restorers for comparison
ast-grep --pattern 'class $_ContentRestorer {
$$$
restore() {
$$$
}
}' packages/loader/src/*Loader.ts
# Look for the ContentRestorer base class or interface
ast-grep --pattern 'class ContentRestorer<$_> {
$$$
}'
Length of output: 1162
Script:
#!/bin/bash
# Search for content restorer implementations with more context
rg "class.*ContentRestorer" -A 10 packages/loader/src/
# Look for the base ContentRestorer definition
rg "class ContentRestorer" -A 10
# Check specific file content
cat packages/loader/src/Texture2DLoader.ts
Length of output: 7463
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
packages/core/src/asset/request.ts (5)
19-24: Consider enhancing type documentation with examplesThe type definition and documentation are well-structured. Consider enhancing it with:
- Example usage
- Descriptions for each property
- Default values documentation
/** * Configuration options for `request`. * @remarks * This type extends the standard `RequestInit` options with additional * properties for handling retries, timeouts, and custom response types. + * @example + * ```typescript + * const config: RequestConfig = { + * type: "json", + * retryCount: 3, + * retryInterval: 1000, + * timeout: 5000 + * }; + * ``` + * @property {XMLHttpRequestResponseType | "image"} [type] - Response type for the request + * @property {number} [retryCount=1] - Number of retry attempts + * @property {number} [retryInterval=500] - Interval between retries in milliseconds + * @property {number} [timeout=Infinity] - Request timeout in milliseconds */
33-36: Enhance function documentation with error handling detailsThe documentation should include information about:
- Error scenarios and how they're handled
- The retry mechanism behavior
- Progress tracking capabilities
/** * Sends a request to the specified URL and returns a promise for the response. * @param url - The URL to send the request to * @param config - Configuration options for the request + * @throws {Error} When the request fails after all retry attempts + * @throws {Error} When the request times out + * @remarks + * The function supports progress tracking through the AssetPromise. + * Retries are attempted based on the retryCount configuration. + * Progress events are emitted for both overall task completion and individual request progress. * @returns A promise that resolves with the response of type `T` */
Line range hint
41-44: Consider setting a reasonable default timeoutUsing
Infinityas the default timeout might lead to hanging requests. Consider setting a reasonable default timeout value (e.g., 30000ms).-const defaultTimeout = Infinity; +const defaultTimeout = 30000; // 30 seconds
Line range hint
116-119: Add error handling for unknown file extensionsThe
getMimeTypeFromUrlfunction should handle cases where the file extension is unknown or unsupported.function getMimeTypeFromUrl(url: string) { const extname = url.substring(url.lastIndexOf(".") + 1); - return mimeType[extname]; + const type = mimeType[extname.toLowerCase()]; + if (!type) { + console.warn(`Unknown file extension: ${extname}. Defaulting to 'text'.`); + return "text"; + } + return type; }
Line range hint
83-86: Ensure blob URL is revoked on timeoutThe blob URL should be revoked when the request times out to prevent memory leaks.
xhr. => { + if (isImg && xhr.response) { + URL.revokeObjectURL(URL.createObjectURL(xhr.response)); + } reject(new Error(`request timeout from: ${url}`)); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/core/src/asset/request.ts(2 hunks)
🔇 Additional comments (1)
packages/core/src/asset/request.ts (1)
21-21: Fix formatting issue detected by linter
An extra space on line 21 should be removed to comply with code style guidelines as indicated by the linter.
Please check if the PR fulfills these requirements
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
What is the current behavior? (You can also link to an open issue here)
What is the new behavior (if this is a feature change)?
Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)
Other information:
Summary by CodeRabbit
Release Notes
New Features
Audioenum value to support audio formats (ogg, wav, mp3).AudioClip,AudioSource, andAudioManagerclasses for enhanced audio management and playback.AudioLoaderfor loading audio assets seamlessly.AudioSourcecomponent is enabled.Bug Fixes
SourceFontenum comment.Tests
AudioSourceto ensure proper functionality and playback behavior.Documentation
requestfunction documentation to clarify its purpose and parameters.