feat: Add videos/images from source with protocol extension #301
Conversation
Review or Edit in CodeSandboxOpen the branch in Web Editor • VS Code • Insiders |
WalkthroughThe changes expand the 3D renderer to support video elements in the scene by introducing a new interface for media properties and multiple methods for video management in the renderer. A set of media channels is registered to control video addition, playback, seeking, volume, speed, and cleanup. Mouse and UI interaction handlers are updated to detect and process video interactions. Additionally, a configuration option for handling cross-origin content has been added. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as "User Client"
participant Channels as "Media Channels Handler"
participant Renderer as "WorldRendererThree"
Client ->> Channels: Send ADD_CHANNEL command with video properties
Channels ->> Renderer: Call destroyMedia(id) if media exists
Channels ->> Renderer: Invoke addMedia(id, props)
Renderer -->> Channels: Media element configured
sequenceDiagram
participant Mouse as "Mouse Event Handler"
participant Channels as "Video Interaction Handler"
participant UI as "React UI Overlay"
Mouse ->> Channels: Check videoCursorInteraction()
alt Video Interaction Detected
Mouse ->> Channels: Call sendVideoInteraction(id, x, y, leftClick?)
UI ->> UI: Display "Interact with video" hint
else No Video Interaction
UI ->> UI: Evaluate cursor state for entity \n and show "Attack" hint if applicable
end
Suggested labels
Poem
✨ Finishing Touches
🪧 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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
PR Code Suggestions ✨Explore these optional code suggestions:
|
|||||||||||||
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
src/customChannels.ts (6)
114-137: Validate unused fields and ensure they don’t cause confusion.
Some fields in theaddPacketStructure(e.g.,_volume,_aspectRatioMode,_background, etc.) are not utilized within the handler. Consider removing them or using them as needed to reduce dead code.
168-192: Potential race condition with default looping.
Whenloopis false, the code setsvideo.loop = falseafteraddVideosetsvideo.loop = trueby default. Although eventually corrected, the initial frames might still seeloop = true.Consider passing the
loopsetting directly intoaddVideoto avoid any momentary mismatch.-worldRenderer.addVideo(id, { +worldRenderer.addVideo(id, { + loop, position: { x, y, z }, ...
225-229: Check “test-video” naming in production.
The function’s name and ID are purely for testing. If intended for production, rename to avoid confusion.
234-249: Consider making video source configurable.
For broader testing, you might accept a dynamic media source instead of the hardcoded'video1.mp4'.
251-251: ExposingaddTestVideoto global scope.
If this is only for debugging, ensure it’s removed or guarded before release.
253-260: Clean up commented-out code.
If this functionality is deprecated or not needed, consider removing altogether to maintain clarity.renderer/viewer/lib/worldrendererThree.ts (1)
477-486: Positional offset approach might be too rigid.
Currently,calculateVideoPositionoffsets only the Z coordinate. If additional rotations/axes are involved in the future, consider making this more generic.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
renderer/viewer/lib/worldrendererThree.ts(3 hunks)src/customChannels.ts(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-and-deploy
🔇 Additional comments (19)
src/customChannels.ts (11)
2-3: Imports look fine.
No obvious issues with importingTHREEorWorldRendererThree.
14-14: Good call registering media channels.
This makes the media functionality available at bot creation.
139-150: Control packet structure is well-defined.
The basic "id" field is clear and minimal, which helps reduce complexity.
152-158: Ensure negative seeks are handled or restricted.
If negative or extreme values for "seconds" are sent, you might get unexpected playback behavior. Consider clamping or validating here.
160-166: Channel registration is consistent.
All channels are registered with the correct packet structures, enabling a straightforward media control flow.
194-199: Playback toggles appear correct.
InvokingsetVideoPlayingis straightforward. Just bear in mind that mobile devices often require a user gesture for autoplay.
201-206: Pause handler is correct.
No concerns found here. The approach is consistent with the "play" handler.
208-213: Seek handler is succinct.
Just confirm no edge cases cause playback errors (e.g., seeking beyond video duration).
215-220: Destroy handler seems robust.
Removes the video from the scene and callsdestroyVideo(), ensuring proper cleanup.
222-223: Debug message is helpful.
The log clarifies success, so this is good for initial diagnostics.
230-233: Destroy old test video cleanly.
This ensures no stale test-video instance remains in the scene.renderer/viewer/lib/worldrendererThree.ts (8)
19-27: Interface appears well-structured.
Clearly defines all essential properties for video rendering.
41-46: Centralized video store is effective.
TheMapprovides straightforward video tracking. Ensure you handle memory disposal promptly when a video is removed.
488-503: Initialize video carefully.
Hardcodingvideo.loop = truemight conflict with the user’sloopintent. Also note that multiple videos each runrequestAnimationFrame(animate), which could cause performance overhead if many videos are added.
504-553: UV mapping support is flexible.
The dynamic UV adjustment is a nice touch. If you anticipate transitions or animations for cropping, ensure speed or performance is handled gracefully.
555-569: Storing video data incustomVideosis good.
The approach centralizes control and maintenance. The method returns theidconsistently for confirmation.
571-580: Simple and straightforward play/pause logic.
No major concerns. Ensure to handle any browser policy restrictions requiring user gestures.
582-587: No boundary checks for seeking.
If the video is short andsecondsis large, ensure you gracefully handle or clamp the value.
589-602: Cleanup is thorough.
This method disposes of texture, geometry, and material. Great job preventing memory leaks.
|
wdyt? @Phoenix616 |
TODO fix rotation
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (8)
src/customChannels.ts (4)
114-137: Media Add Channel structure requires optimization for future extensibility.The packet structure includes several parameters prefixed with underscore (_) that appear to be placeholder values. While this approach allows for future expansion, consider the following improvements:
- Group related parameters into structured objects
- Consider using versioned packet structures for better backward compatibility
- Document the purpose of underscore-prefixed parameters
const addPacketStructure = [ 'container', [ { name: 'id', type: ['pstring', { countType: 'i16' }] }, { name: 'x', type: 'i32' }, { name: 'y', type: 'i32' }, { name: 'z', type: 'i32' }, { name: 'width', type: 'f32' }, { name: 'height', type: 'f32' }, { name: 'rotation', type: 'u8' }, // 0-3 for 0°, 90°, 180°, 270° (3-6 is same but double side) { name: 'source', type: ['pstring', { countType: 'i16' }] }, { name: 'loop', type: 'bool' }, - { name: '_volume', type: 'f32' }, // 0 - { name: '_aspectRatioMode', type: 'u8' }, // 0 - { name: '_background', type: 'u8' }, // 0 - { name: '_opacity', type: 'u8' }, // 1 - { name: '_cropXStart', type: 'f32' }, // 0 - { name: '_cropYStart', type: 'f32' }, // 0 - { name: '_cropXEnd', type: 'f32' }, // 0 - { name: '_cropYEnd', type: 'f32' }, // 0 + { name: 'options', type: ['pstring', { countType: 'i16' }] }, // JSON string for additional options ] ]
195-200: Consider implementing response mechanism for media control operations.The current implementation doesn't provide any feedback on whether control operations (play, pause, seek, destroy) were successful. For robust client-server communication, consider adding a response or acknowledgment mechanism.
A feedback mechanism could be implemented by adding a callback channel or using promises to handle success/failure states. This would allow the server to know if the operation succeeded.
Also applies to: 202-207, 209-214, 216-221
226-251: Test function exposed globally creates potential namespace pollution.The
addTestVideofunction is exposed directly to the window object, which could lead to namespace conflicts in a production environment. Consider a more isolated approach for testing functionality.const addTestVideo = (rotation = 0 as 0 | 1 | 2 | 3, scale = 1) => { // Function implementation } - window.addTestVideo = addTestVideo + // Only expose in development mode + if (process.env.NODE_ENV === 'development') { + (window as any).addTestVideo = addTestVideo + }
254-261: Remove commented-out code before merging.There's commented-out code for a
registerMediaOpenChannelfunction that appears incomplete. Either complete and integrate this functionality or remove it from the codebase.Remove the commented-out incomplete code to maintain a clean codebase.
renderer/viewer/lib/worldrendererThree.ts (4)
477-500: Video positioning calculation needs validation logic.The
calculateVideoPositionmethod handles rotation properly, but lacks validation for edge cases. Consider adding validation for input parameters and handling potential out-of-range situations.private calculateVideoPosition (position: { x: number, y: number, z: number }, side: 'towards' | 'away', rotation?: 0 | 1 | 2 | 3): { x: number, y: number, z: number } { + // Validate inputs + if (rotation !== undefined && (rotation < 0 || rotation > 3)) { + console.warn('Invalid rotation value:', rotation, 'using default 0') + rotation = 0 + } const offset = side === 'towards' ? 0.999 : 0.001 // Position is bottom-left corner, so we need to add half the size to center it // Only apply towards/away offset to Z-axis, normalize X/Y with 0.5 const baseX = Math.floor(position.x) + 0.5 const baseY = Math.floor(position.y) + 0.5 const baseZ = Math.floor(position.z) + (side === 'towards' ? 1 : 0) - offset // Adjust position based on rotation to maintain correct starting point if (rotation !== undefined) { // Rest of the code remains the same } return { x: baseX, y: baseY, z: baseZ } }
581-591: Consider implementing an event system for video state changes.The current implementation lacks a way to notify other parts of the application about video state changes. Adding an event system would improve integration with UI components or other features.
// Add to class definition + videoEvents = new EventTarget() // In addVideo method this.customVideos.set(id, { mesh: group, video, texture: videoTexture, updateUVMapping }) + // Dispatch event + this.videoEvents.dispatchEvent(new CustomEvent('videoAdded', { detail: { id } })) return id
593-602: Add return value to indicate success of video control operations.The
setVideoPlayingmethod doesn't return any value to indicate success or failure. Consider adding a return value to help callers handle error cases.- setVideoPlaying (id: string, playing: boolean) { + setVideoPlaying (id: string, playing: boolean): boolean { const videoData = this.customVideos.get(id) if (videoData) { if (playing) { - videoData.video.play().catch(console.error) + return videoData.video.play() + .then(() => true) + .catch(err => { + console.error(`Error playing video ${id}:`, err) + return false + }) } else { videoData.video.pause() + return true } } + return false }
604-609: Add range validation for seeking operations.The
setVideoSeekingmethod doesn't validate if the requested time is within the video duration, which could lead to seeking to invalid positions.setVideoSeeking (id: string, seconds: number) { const videoData = this.customVideos.get(id) if (videoData) { + // Validate time range + const duration = videoData.video.duration + if (isNaN(duration)) { + console.warn(`Cannot seek video ${id} - duration unknown`) + return + } + seconds = Math.max(0, Math.min(seconds, duration)) videoData.video.currentTime = seconds } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
renderer/viewer/lib/worldrendererThree.ts(3 hunks)src/customChannels.ts(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-and-deploy
🔇 Additional comments (6)
src/customChannels.ts (2)
2-3: Proper imports added for handling media functionality.The necessary import of THREE.js and the WorldRendererThree class has been added, which are required for the new video functionality.
14-14: Registration of media channels properly hooked into initialization flow.The media channels registration function is correctly called during bot creation, consistent with the existing pattern used for block models channel registration.
renderer/viewer/lib/worldrendererThree.ts (4)
19-27: Well-defined interface for video properties.The
VideoPropertiesinterface provides a clear contract for video integration with appropriate optional properties. This allows for flexible video configuration while maintaining type safety.
41-46: Customized video storage with proper typing.The
customVideosMap is well-structured with appropriate types for storing video-related data including meshes, video elements, and textures. The inclusion of a function to update UV mapping provides a clean API for texture manipulation.
563-580: UV mapping implementation is robust.The implementation of UV mapping for videos is well-designed, allowing for dynamic texture coordinate adjustment. This provides flexibility for features like video cropping and texture transforms.
611-630: Thorough cleanup in destroyVideo method.The
destroyVideomethod properly cleans up all resources associated with a video, preventing memory leaks. This includes pausing the video, clearing its source, removing it from the scene, and disposing of textures and geometry.
|
/deploy |
|
Deployed to Vercel Preview: https://prismarine-1k1iy6rw2-zaro.vercel.app |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/customChannels.ts (1)
168-193:⚠️ Potential issueAdd error handling for media operations.
The current implementation lacks error handling for potential issues like invalid URLs or playback failures. Media operations can fail due to various reasons including network issues, unsupported formats, or CORS restrictions.
bot._client.on(ADD_CHANNEL as any, (data) => { const { id, x, y, z, width, height, rotation, source, loop } = data const worldRenderer = viewer.world as WorldRendererThree // Destroy existing video if it exists worldRenderer.destroyVideo(id) + try { // Add new video worldRenderer.addVideo(id, { position: { x, y, z }, size: { width, height }, side: 'towards', src: source, rotation: rotation as 0 | 1 | 2 | 3, doubleSide: false }) // Set loop state if (!loop) { const videoData = worldRenderer.customVideos.get(id) if (videoData) { videoData.video.loop = false } } + } catch (error) { + console.error(`Failed to add video ${id}:`, error) + } })
🧹 Nitpick comments (5)
src/customChannels.ts (5)
126-126: Clarify the rotation parameter comment.The comment states rotation values 3-6 are for double side, but in the implementation (line 183), doubleSide is explicitly set to false, and line 182 only casts to 0-3. This creates a potential confusion for users of this API.
- { name: 'rotation', type: 'u8' }, // 0-3 for 0°, 90°, 180°, 270° (3-6 is same but double side) + { name: 'rotation', type: 'u8' }, // 0-3 for 0°, 90°, 180°, 270°
129-136: Document the purpose of underscore-prefixed fields.These fields appear to be placeholders or future parameters that aren't currently being used in the implementation. Consider adding a more descriptive comment explaining their purpose or implementing them if they're intended to be used.
171-171: Add type checking before casting to WorldRendererThree.The code assumes viewer.world is an instance of WorldRendererThree without verification. Add a check to prevent runtime errors if a different renderer is used.
-const worldRenderer = viewer.world as WorldRendererThree +const worldRenderer = viewer.world +if (!(worldRenderer instanceof WorldRendererThree)) { + console.error('Current world renderer does not support video functionality') + return +}
226-251: Improve the test video function implementation.The test function has several improvement opportunities:
- The video URL is hardcoded and might not always be available
- The function is exposed globally without any checks
- Missing error handling
-const addTestVideo = (rotation = 0 as 0 | 1 | 2 | 3, scale = 1) => { +const addTestVideo = (rotation = 0 as 0 | 1 | 2 | 3, scale = 1, videoUrl = 'https://threejs.org/examples/textures/sintel.mp4') => { const block = window.cursorBlockRel() if (!block) return const { position: startPosition } = block const worldRenderer = viewer.world as WorldRendererThree + if (!(worldRenderer instanceof WorldRendererThree)) { + console.error('Current world renderer does not support video functionality') + return + } + + try { worldRenderer.destroyVideo('test-video') // Add video with proper positioning worldRenderer.addVideo('test-video', { position: { x: startPosition.x, y: startPosition.y, z: startPosition.z }, size: { width: scale, height: scale }, side: 'towards', - src: 'https://threejs.org/examples/textures/sintel.mp4', + src: videoUrl, rotation, doubleSide: true }) + } catch (error) { + console.error('Failed to add test video:', error) + } } -window.addTestVideo = addTestVideo +// Only expose in development environment +if (process.env.NODE_ENV !== 'production') { + window.addTestVideo = addTestVideo +}
114-224: Consider implementing a validation layer for received data.The current implementation assumes that all data received through channels is valid. Adding validation would improve robustness and help identify potential issues with the protocol or client implementation.
You could implement a validation function for each packet type:
function validateAddData(data: any): boolean { if (!data.id || typeof data.id !== 'string') return false; if (typeof data.x !== 'number' || typeof data.y !== 'number' || typeof data.z !== 'number') return false; if (typeof data.width !== 'number' || typeof data.height !== 'number') return false; if (typeof data.rotation !== 'number' || data.rotation < 0 || data.rotation > 3) return false; if (!data.source || typeof data.source !== 'string') return false; return true; } // Then use in handler: bot._client.on(ADD_CHANNEL as any, (data) => { if (!validateAddData(data)) { console.error('Invalid data received for media add:', data); return; } // Rest of handler code... });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/customChannels.ts(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-and-deploy
🔇 Additional comments (2)
src/customChannels.ts (2)
2-3: LGTM: Imports are appropriate for the new functionality.The THREE.js library and WorldRendererThree class are correctly imported to support the new video rendering capabilities.
14-14: LGTM: Media channels registration added to bot initialization.The new function is properly called within the existing bot initialization flow.
|
/deploy |
|
Deployed to Vercel Preview: https://prismarine-jze2rqbgz-zaro.vercel.app |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
renderer/viewer/lib/worldrendererThree.ts (1)
502-592: Consolidate position logic to avoid code duplication.The code for computing block center and offset in lines 534-539 mirrors what you introduced in
calculateVideoPosition(). Unifying these computations would help maintain consistency, reduce mistakes, and ease future changes.Here’s a minimal diff example that replaces the inline calculation with
calculateVideoPosition():--- Before +-- After @@ -533,14 +477,7 @@ const group = new THREE.Group() group.add(mesh) -// Calculate base position (center of block) -const baseX = Math.floor(props.position.x) + 0.5 -const baseY = Math.floor(props.position.y) + 0.5 -const baseZ = Math.floor(props.position.z) + (props.side === 'towards' ? 1 : 0) - (props.side === 'towards' ? 0.999 : 0.001) - -group.position.set(baseX, baseY, baseZ) - -// ... +const calculated = this.calculateVideoPosition(props.position, props.side, props.rotation) +group.position.set(calculated.x, calculated.y, calculated.z) // Then remove the duplicated rotation code if it's handled within calculateVideoPosition or unify the approach
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
renderer/viewer/lib/worldrendererThree.ts(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-and-deploy
🔇 Additional comments (4)
renderer/viewer/lib/worldrendererThree.ts (4)
19-27: Well-defined interface.These properties comprehensively capture the requirements for video rendering. Nice work ensuring flexibility with optional fields like
rotation,doubleSide, anduvMapping.
41-46: Straightforward map approach.Storing all video resources in a centralized
Mapis clear and maintainable, facilitating easy retrieval and cleanup.
477-500: Potentially unused helper function.
calculateVideoPositiondoesn’t appear to be called in this file, and its logic is separately re-implemented inaddVideo(). To keep the codebase lean and consistent, either remove it or integrate it intoaddVideo()to avoid duplication.If it’s intentionally reserved for future usage, please confirm; otherwise, consider removing it.
555-561: Performance concern with per-video animation loops.Each video spawns its own
requestAnimationFrameloop, which can degrade performance when handling multiple videos. As previously noted in past reviews, consider consolidating these updates in a single animation loop to optimize performance.
Looks good to me. How smooth is the video? Just like on a normal website or is there some limitation on the frame rate? (besides the actual client renderer FPS of course) |
This is a video texture. It's just a simple image texture that is updated on every render call so obviously video can have any framerate (but since its web videos with 120+ fps might take more time to load over network) |
|
test_video.mp4 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
src/customChannels.ts (2)
116-173: Good structured channel registration, consider minimal validation.
All relevant packet structures are neatly described and registered. You could add minimal validation (e.g. numeric range checks) to guard against malformed data.+ // Example: limit 'width', 'height' to sane ranges + if (data.width < 0 || data.height < 0) { + throw new Error("Invalid media dimensions") + }
310-344: Useful test function, consider removing in production.
addTestVideois helpful for debugging, but ensure it’s gated or removed in production to avoid exposing test data.renderer/viewer/lib/worldrendererThree.ts (3)
504-553: Use consistent text styling for error and background textures.
The code is good for fallback visuals. For a more uniform look, ensure the text font size and opacity match your design guidelines across all fallback textures.
555-665: Captures errors gracefully, but watch for autoplay restrictions.
Thevideo.play().catch(...)block is correct for logging errors. Note that some browsers block autoplay. Provide a fallback or user action for enabling playback if necessary.I can provide code to handle user gestures if desired; would you like me to open an issue for that?
695-746: Lifecycle management is good, consider unsubscribing or removing all references.
destroyVideoremoves the mesh, texture, and video, but also ensure no leftover references in the broader application, e.g., external event listeners.+ // Example cleanup in destroyVideo + videoData.video.removeAttribute('src') + videoData.video.load()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
renderer/viewer/lib/worldrendererThree.ts(3 hunks)src/customChannels.ts(3 hunks)src/index.ts(1 hunks)src/mineflayer/plugins/mouse.ts(2 hunks)src/optionsStorage.ts(1 hunks)src/react/GameInteractionOverlay.tsx(2 hunks)src/react/TouchInteractionHint.tsx(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-and-deploy
🔇 Additional comments (17)
src/optionsStorage.ts (1)
62-62: Good addition for feature configurationThe new
networkRelatedCustomChannelsproperty provides a way to toggle network-related custom channel functionality, which aligns well with the video rendering capabilities being introduced in this PR.src/mineflayer/plugins/mouse.ts (2)
25-25: Appropriate imports for video interaction functionalityThe imports for
sendVideoInteractionandvideoCursorInteractionare necessary for the new video interaction features being implemented.
184-188: Good prioritization of video interactionsThis code correctly checks for video interactions first and handles them appropriately before falling back to standard mouse click handling. This prioritization ensures videos are properly interactive in the 3D scene.
src/react/GameInteractionOverlay.tsx (2)
8-8: Appropriate import for video cursor interactionThe import of
videoCursorInteractionis necessary for the extended functionality.
157-157: Good enhancement of attack conditionThe condition for
gonnaAttackhas been correctly extended to include video interactions, ensuring that both entities and videos can be interacted with using the same input mechanism.src/react/TouchInteractionHint.tsx (2)
5-5: Necessary import for video cursor interactionThis import enables the touch interaction hint system to recognize video elements.
18-29: Well-structured UI hint logic for video interactionsThe code has been restructured in a clear and logical way to:
- First check for video interactions
- Show an appropriate "Interact with video" hint when needed
- Fall back to entity-based hints if no video is being interacted with
This change maintains a consistent user experience by providing contextual hints for all interactive elements.
src/index.ts (1)
897-898: Confirm correctness of the newsubscribeusage.
Switching fromwatchValuetosubscribeappears valid, and you cleanly handle unsubscription right afterfsReadyandappConfigare set. This prevents potential leaks.src/customChannels.ts (6)
14-16: Conditional registration check seems fine.
The logic ensures media channels are only registered ifoptions.networkRelatedCustomChannelsis true, preventing unnecessary overhead. No concerns here.
175-208: Ensure robust error handling for unsupported images.
Rejecting image files is clear. However, adding a try-catch block to guard against other unexpected failures (e.g., invalidrotationor network errors) is beneficial.
209-249: Add error handling for media control operations.
As in prior suggestions, wrapping each video operation in a try-catch block helps handle unexpected failures gracefully.
275-307: Raycasting logic is efficient, but watch out for camera or mesh changes.
This center-screen approach works, yet if you expand to multiple cameras or off-center interactions, you'll need to adapt.
253-273:❓ Verification inconclusive
Confirm interaction channel usage.
TheMEDIA_INTERACTION_CHANNELandsendVideoInteractionmethod look correct, but re-check boundary flows: e.g., what ifiddoesn't exist incustomVideos.
🏁 Script executed:
#!/bin/bash # Verify calls to sendVideoInteraction always reference existing video IDs rg -A 5 "sendVideoInteraction"Length of output: 1273
Action Required: Validate Video ID Usage in Interaction Channel
The registration of the interaction channel and the
sendVideoInteractionmethod appears correct. However, please verify that every invocation ofsendVideoInteractionuses a valid video ID fromcustomVideos. For example, insrc/mineflayer/plugins/mouse.tsthe function is invoked withvideoInteraction.idwithout any safeguard confirming that this ID exists incustomVideos. Consider adding explicit validation or boundary checks to handle cases where an invalid or missing ID might be passed.
2-3:✅ Verification successful
Validate imports for concurrency.
You're importingTHREEandWorldRendererThreeat the top-level scope. Ensure that any references toviewer.worldorbotthroughout the file are always ready to consume these imports safely.Please confirm you never call
worldRendererorviewer.worldbefore they are actually initialized by verifying all references:
🏁 Script executed:
#!/bin/bash # Search for references to `viewer.world` or `worldRenderer` before custom channels registration. rg -A 5 "viewer.world|worldRenderer"Length of output: 49974
Concurrency Safety Verified
After a comprehensive review of the codebase—especially the callbacks and event handlers in src/customChannels.ts—all references to
viewer.world(and its aliasworldRenderer) occur only within function bodies or event listener callbacks that are invoked after the viewer is fully initialized. There are no instances where these references are called at the module level or before the proper initialization routines are complete. The usage is consistent with the safe patterns used throughout other parts of the repository.renderer/viewer/lib/worldrendererThree.ts (3)
19-29: Interface design forVideoPropertieslooks clear.
Properties are well-documented. The optionaluvMappingis a nice touch for custom cropping. No issues found.
479-502: Check correctness of positional offset logic.
Your coordinate adjustments forsideandrotationlook sensible but confirm with in-game testing that the final alignment meets the intended orientation.
668-671: Position scaling method is short and clear.
This helper ensures consistent application of(width, height, depth). No issues found.
|
Have no idea how to fix rotation and scale, uv (to preserve aspect ratio) and opacity |
|
/deploy |
|
Deployed to Vercel Preview: https://prismarine-cxas0s3dj-zaro.vercel.app |
|
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (7)
src/customChannels.ts (7)
175-207:⚠️ Potential issueAdd error handling for media add operations and fix rotation handling.
The current implementation throws an error when image files are detected but doesn't catch it, which could crash the application. Also, the rotation parameter is ignored and hardcoded to 0, contradicting the comment in the packet structure.
bot._client.on(ADD_CHANNEL as any, (data) => { const { id, x, y, z, width, height, rotation, source, loop, background, opacity } = data + try { if (source.endsWith('.png') || source.endsWith('.jpg') || source.endsWith('.jpeg')) { - throw new Error('Image files are not supported yet, please use Minecraft maps instead') + console.error('Image files are not supported yet, please use Minecraft maps instead') + return; } const worldRenderer = viewer.world as WorldRendererThree // Destroy existing video if it exists worldRenderer.destroyVideo(id) // Add new video worldRenderer.addVideo(id, { position: { x, y, z }, size: { width, height }, side: 'towards', src: source, - // rotation: rotation as 0 | 1 | 2 | 3, - rotation: 0, + rotation: rotation as 0 | 1 | 2 | 3, doubleSide: false, background, opacity: opacity / 100 }) // Set loop state if (!loop) { const videoData = worldRenderer.customVideos.get(id) if (videoData) { videoData.video.loop = false } } + } catch (error) { + console.error(`Failed to add video ${id}:`, error) + } })
209-214:⚠️ Potential issueAdd error handling for play operation.
Similar to the Add handler, this operation lacks error handling. Wrap the operation in a try-catch block to gracefully handle failures.
bot._client.on(PLAY_CHANNEL as any, (data) => { const { id } = data const worldRenderer = viewer.world as WorldRendererThree + try { worldRenderer.setVideoPlaying(id, true) + } catch (error) { + console.error(`Failed to play video ${id}:`, error) + } })
217-221:⚠️ Potential issueAdd error handling for pause operation.
Add try-catch block to handle potential failures when pausing a video.
bot._client.on(PAUSE_CHANNEL as any, (data) => { const { id } = data const worldRenderer = viewer.world as WorldRendererThree + try { worldRenderer.setVideoPlaying(id, false) + } catch (error) { + console.error(`Failed to pause video ${id}:`, error) + } })
224-228:⚠️ Potential issueAdd error handling for seek operation and update parameter name.
Add try-catch block and update the parameter name to match the packet structure.
bot._client.on(SEEK_CHANNEL as any, (data) => { - const { id, seconds } = data + const { id, value } = data const worldRenderer = viewer.world as WorldRendererThree + try { - worldRenderer.setVideoSeeking(id, seconds) + worldRenderer.setVideoSeeking(id, value) + } catch (error) { + console.error(`Failed to seek video ${id}:`, error) + } })
231-235:⚠️ Potential issueAdd error handling for destroy operation.
Add try-catch block to handle potential failures when destroying a video.
bot._client.on(DESTROY_CHANNEL as any, (data) => { const { id } = data const worldRenderer = viewer.world as WorldRendererThree + try { worldRenderer.destroyVideo(id) + } catch (error) { + console.error(`Failed to destroy video ${id}:`, error) + } })
238-242:⚠️ Potential issueAdd error handling for volume operation and update parameter name.
Add try-catch block and update the parameter name to match the packet structure.
bot._client.on(VOLUME_CHANNEL as any, (data) => { - const { id, volume } = data + const { id, value } = data const worldRenderer = viewer.world as WorldRendererThree + try { - worldRenderer.setVideoVolume(id, volume) + worldRenderer.setVideoVolume(id, value) + } catch (error) { + console.error(`Failed to set volume for video ${id}:`, error) + } })
245-249:⚠️ Potential issueAdd error handling for speed operation and update parameter name.
Add try-catch block and update the parameter name to match the packet structure.
bot._client.on(SPEED_CHANNEL as any, (data) => { - const { id, speed } = data + const { id, value } = data const worldRenderer = viewer.world as WorldRendererThree + try { - worldRenderer.setVideoSpeed(id, speed) + worldRenderer.setVideoSpeed(id, value) + } catch (error) { + console.error(`Failed to set speed for video ${id}:`, error) + } })
🧹 Nitpick comments (3)
src/customChannels.ts (3)
157-163: Improve parameter naming in setNumberPacketStructure.The parameter name
secondsis used for both time-related operations (seek) and non-time-related operations (volume, speed). A more generic name likevaluewould be more appropriate.const setNumberPacketStructure = [ 'container', [ { name: 'id', type: ['pstring', { countType: 'i16' }] }, - { name: 'seconds', type: 'f32' } + { name: 'value', type: 'f32' } ] ]
269-273: Add validation to sendVideoInteraction function.The function should validate input parameters to prevent potential errors.
export const sendVideoInteraction = (id: string, x: number, y: number, isRightClick: boolean) => { + if (!id) { + console.error('Video ID is required for interaction') + return + } + + if (isNaN(x) || isNaN(y)) { + console.error('Invalid coordinates for video interaction') + return + } bot._client.writeChannel(MEDIA_INTERACTION_CHANNEL, { id, x, y, isRightClick }) }
275-307: Add error handling to videoCursorInteraction function.The function should handle potential errors that might occur during raycasting or when accessing objects that might not be fully initialized.
export const videoCursorInteraction = () => { + try { const worldRenderer = viewer.world as WorldRendererThree + if (!worldRenderer || !worldRenderer.camera) { + return null + } const { camera } = worldRenderer const raycaster = new THREE.Raycaster() // Get mouse position at center of screen const mouse = new THREE.Vector2(0, 0) // Update the raycaster raycaster.setFromCamera(mouse, camera) // Check intersection with all video meshes for (const [id, videoData] of worldRenderer.customVideos.entries()) { // Get the actual mesh (first child of the group) const mesh = videoData.mesh.children[0] as THREE.Mesh if (!mesh) continue const intersects = raycaster.intersectObject(mesh, false) if (intersects.length > 0) { const intersection = intersects[0] const { uv } = intersection if (!uv) return null return { id, x: uv.x, y: uv.y } } } return null + } catch (error) { + console.error('Error in videoCursorInteraction:', error) + return null + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/customChannels.ts(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-and-deploy
🔇 Additional comments (2)
src/customChannels.ts (2)
2-3: Appropriate imports for THREE.js integration.Good addition of necessary imports for the new video functionality. The THREE.js library is essential for the 3D rendering capabilities.
14-16: Good conditional registration of media channels.Wrapping the media channel registration inside the
options.networkRelatedCustomChannelscheck is a good practice, as it provides a way to disable this feature when not needed.
|
/deploy |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
src/customChannels.ts (4)
304-336: 🛠️ Refactor suggestionAdd error handling to addTestVideo and address TODO comment.
The function contains a TODO comment about broken UV mapping that should be addressed or documented better. Additionally, add error handling to gracefully manage failures.
const addTestVideo = (rotation = 0 as 0 | 1 | 2 | 3, scale = 1, isImage = false) => { + try { const block = window.cursorBlockRel() if (!block) return const { position: startPosition } = block const worldRenderer = viewer.world as WorldRendererThree // Add video with proper positioning worldRenderer.addMedia('test-video', { position: { x: startPosition.x, y: startPosition.y + 1, z: startPosition.z }, size: { width: scale, height: scale }, src: isImage ? 'https://bucket.mcraft.fun/test_image.png' : 'https://bucket.mcraft.fun/test_video.mp4', rotation, // doubleSide: true, background: 0x00_00_00, // Black color // TODO broken // uvMapping: { // startU: 0, // endU: 1, // startV: 0, // endV: 1 // }, opacity: 1, allowOrigins: true, }) + } catch (error) { + console.error('Error in addTestVideo:', error) + } }Also, consider creating a GitHub issue to track the broken UV mapping functionality mentioned in the TODO comment.
119-137:⚠️ Potential issueFix field naming inconsistency in packet structure.
There's an inconsistency between the field names in the packet structure (prefixed with underscore) and their usage in the handler (without underscore). For example,
_backgroundis defined but accessed asbackgroundin line 189.- { name: '_volume', type: 'f32' }, // 0 + { name: 'volume', type: 'f32' }, // 0 - { name: '_aspectRatioMode', type: 'u8' }, // 0 + { name: 'aspectRatioMode', type: 'u8' }, // 0 - { name: '_background', type: 'u8' }, // 0 + { name: 'background', type: 'u8' }, // 0 - { name: '_opacity', type: 'u8' }, // 1 + { name: 'opacity', type: 'u8' }, // 1 - { name: '_cropXStart', type: 'f32' }, // 0 + { name: 'cropXStart', type: 'f32' }, // 0 - { name: '_cropYStart', type: 'f32' }, // 0 + { name: 'cropYStart', type: 'f32' }, // 0 - { name: '_cropXEnd', type: 'f32' }, // 0 + { name: 'cropXEnd', type: 'f32' }, // 0 - { name: '_cropYEnd', type: 'f32' }, // 0 + { name: 'cropYEnd', type: 'f32' }, // 0
203-243: 🛠️ Refactor suggestionAdd error handling for media control operations.
Similar to the Add handler, these control operations lack error handling. Wrap the operations in try-catch blocks to gracefully handle failures.
// Example for one handler (apply similar pattern to all): bot._client.on(PLAY_CHANNEL as any, (data) => { const { id } = data const worldRenderer = viewer.world as WorldRendererThree + try { worldRenderer.setVideoPlaying(id, true) + } catch (error) { + console.error(`Failed to play video ${id}:`, error) + } })
173-201: 🛠️ Refactor suggestionAdd error handling for media operations.
The current implementation lacks error handling for potential issues like invalid URLs or playback failures. Media operations can fail due to various reasons including network issues, unsupported formats, or CORS restrictions.
bot._client.on(ADD_CHANNEL as any, (data) => { const { id, x, y, z, width, height, rotation, source, loop, background, opacity } = data const worldRenderer = viewer.world as WorldRendererThree // Destroy existing video if it exists worldRenderer.destroyMedia(id) + try { // Add new video worldRenderer.addMedia(id, { position: { x, y, z }, size: { width, height }, // side: 'towards', src: source, rotation: rotation as 0 | 1 | 2 | 3, doubleSide: false, background, opacity: opacity / 100, allowOrigins: options.remoteContentNotSameOrigin === false ? [getCurrentTopDomain()] : options.remoteContentNotSameOrigin }) // Set loop state if (!loop) { const videoData = worldRenderer.customMedia.get(id) if (videoData?.video) { videoData.video.loop = false } } + } catch (error) { + console.error(`Failed to add media ${id}:`, error) + } })
🧹 Nitpick comments (6)
src/customChannels.ts (1)
260-260: Add channel registration status to logs.Good logging practice includes listing all registered channels in debug output. Consider enhancing the log message to include all registered channel names.
- console.debug('Registered media channels') + console.debug(`Registered media channels: ${ADD_CHANNEL}, ${PLAY_CHANNEL}, ${PAUSE_CHANNEL}, ${SEEK_CHANNEL}, ${VOLUME_CHANNEL}, ${SPEED_CHANNEL}, ${DESTROY_CHANNEL}, ${MEDIA_INTERACTION_CHANNEL}`)renderer/viewer/lib/worldrendererThree.ts (5)
562-674: Performance concern with individual animation loops.Each video gets its own
onBeforeRendercallback, which can lead to performance issues when multiple videos are present. Consider consolidating these into a single animation loop.addMedia (id: string, props: MediaProperties) { // Create video element and other setup code... if (video) { - // Update texture in animation loop - mesh.onBeforeRender = () => { - if (video.readyState === video.HAVE_ENOUGH_DATA) { - if (material.map !== texture) { - material.map = texture - material.needsUpdate = true - } - texture.needsUpdate = true - } - } + // Add to tracking set for centralized animation + this._videosToUpdate.add({ + id, + update: () => { + if (video.readyState === video.HAVE_ENOUGH_DATA) { + if (material.map !== texture) { + material.map = texture + material.needsUpdate = true + } + texture.needsUpdate = true + } + } + }) } // Rest of the function... } + // Add these to the class: + private _videosToUpdate = new Set<{id: string, update: () => void}>() + private _videoUpdateLoop = () => { + // Update all video textures that need updating + for (const item of this._videosToUpdate) { + item.update() + } + requestAnimationFrame(this._videoUpdateLoop) + } + + // Initialize the loop in the constructor + constructor(...) { + // existing code + this._videoUpdateLoop() + }
676-685: Add timeout and error handling to video playback control.When setting video playback state, add timeout handling to prevent hanging on slow network conditions.
setVideoPlaying (id: string, playing: boolean) { const videoData = this.customMedia.get(id) if (videoData?.video) { if (playing) { - videoData.video.play().catch(console.error) + const playPromise = videoData.video.play() + if (playPromise !== undefined) { + const timeoutPromise = new Promise((_, reject) => { + setTimeout(() => reject(new Error('Video play timeout')), 5000) + }) + + Promise.race([playPromise, timeoutPromise]) + .catch(error => { + console.error(`Failed to play video ${id}:`, error) + }) + } } else { videoData.video.pause() } } }
743-848: Simplify the debug visualization in positionMeshExact.The positioning logic is solid, but the debug visualization with multiple markers should be conditionally enabled based on a debug flag to avoid cluttering the scene in production.
positionMeshExact ( mesh: THREE.Mesh, rotation: number, startPosition: { x: number, y: number, z: number }, width: number, height: number, depth = 1 ) { // ... existing positioning code ... // Create a group to hold our mesh and markers const debugGroup = new THREE.Group() debugGroup.add(mesh) + // Only add debug markers when in debug mode + const showDebugMarkers = false; // Change to a class property or config option + if (showDebugMarkers) { // Add a marker at the starting position (should be exactly at pos) const startMarker = new THREE.Mesh( new THREE.BoxGeometry(0.1, 0.1, 0.1), new THREE.MeshBasicMaterial({ color: 0xff_00_00 }) ) startMarker.position.copy(new THREE.Vector3(startPosition.x, startPosition.y, startPosition.z)) debugGroup.add(startMarker) // ... other debug markers and visualization ... + } return { mesh, debugGroup } }
849-859: Remove or refactor unused test function.The
createTestCanvasTexturefunction appears to be used only for testing. Consider removing it or refactoring it to be more generic if needed for production.-createTestCanvasTexture () { +// Removed or moved to a testing utility file
864-887: Make addTestMeshExact a testing utility function.This function is purely for testing/debugging purposes. Consider moving it to a separate testing utilities file or make it conditionally available only in development mode.
-addTestMeshExact (rotationNum: number) { +// Removed or moved to a testing utility file
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
renderer/viewer/lib/worldrendererThree.ts(3 hunks)src/customChannels.ts(3 hunks)src/optionsStorage.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/optionsStorage.ts
🔇 Additional comments (10)
src/customChannels.ts (3)
114-144: Good channel organization and naming.The media channels are well-organized with clear, descriptive names that follow a consistent pattern. The separation into distinct channels for different operations (add, play, pause, seek, etc.) follows good design principles.
269-301: Well-implemented cursor interaction detection.The
videoCursorInteractionfunction efficiently detects interactions with video meshes using raycasting. This is a clean implementation that properly handles the 3D to 2D mapping via UV coordinates.
339-358: Well-implemented domain extraction logic.The
getCurrentTopDomainfunction properly handles various domain formats, including special cases like country-specific domains (co.uk, com.br, etc.). This ensures proper origin security checks.renderer/viewer/lib/worldrendererThree.ts (7)
19-29: Well-structured MediaProperties interface.The interface is comprehensive and includes all necessary properties for media handling, with appropriate typing and documentation. The optional properties are correctly defined with question marks.
43-48: Good data structure for managing media elements.Using a Map for storing custom media elements with a well-defined value type is a good approach. It provides efficient lookup by ID while maintaining all necessary properties and methods for each media element.
479-502: Good position calculation logic with rotation support.The
calculateVideoPositionmethod handles different rotation cases properly, ensuring correct positioning of media elements in the 3D space regardless of their rotation angle.
504-531: Well-implemented error texture for fallback scenarios.Creating a visual indicator when media fails to load is a good UX practice. The error texture implementation is thorough with proper scaling and text rendering.
533-553: Efficient background texture implementation.Using a 1x1 canvas for solid color backgrounds is memory-efficient. The color conversion from hex to RGBA components is also well-implemented.
647-664: Well-implemented UV mapping configuration.The UV mapping implementation allows for fine-grained control over texture coordinates, enabling features like cropping and custom aspect ratios. The update function is well-designed to modify all UV coordinates properly.
708-729: Thorough resource cleanup in destroyMedia.The
destroyMediamethod properly handles cleanup of all resources including video elements, meshes, textures, and geometries. This is important to prevent memory leaks when videos are frequently added and removed.
| if (video) { | ||
| // Start playing the video | ||
| video.play().catch(err => { | ||
| console.error('Failed to play video:', err) | ||
| handleError() | ||
| }) | ||
|
|
||
| // Update texture in animation loop | ||
| mesh.onBeforeRender = () => { | ||
| if (video.readyState === video.HAVE_ENOUGH_DATA) { | ||
| if (material.map !== texture) { | ||
| material.map = texture | ||
| material.needsUpdate = true | ||
| } | ||
| texture.needsUpdate = true | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add better error handling for video playback.
The current error handling for video playback is minimal. Consider adding more robust error handling and recovery mechanisms.
if (video) {
// Start playing the video
video.play().catch(err => {
console.error('Failed to play video:', err)
handleError()
+ // Implement retry logic or more detailed error feedback
+ if (err.name === 'NotAllowedError') {
+ console.warn('Autoplay prevented by browser. User interaction required.')
+ } else if (err.name === 'NotSupportedError') {
+ console.error('Video format not supported by this browser')
+ }
})
// ...
}📝 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.
| if (video) { | |
| // Start playing the video | |
| video.play().catch(err => { | |
| console.error('Failed to play video:', err) | |
| handleError() | |
| }) | |
| // Update texture in animation loop | |
| mesh.onBeforeRender = () => { | |
| if (video.readyState === video.HAVE_ENOUGH_DATA) { | |
| if (material.map !== texture) { | |
| material.map = texture | |
| material.needsUpdate = true | |
| } | |
| texture.needsUpdate = true | |
| } | |
| } | |
| } | |
| if (video) { | |
| // Start playing the video | |
| video.play().catch(err => { | |
| console.error('Failed to play video:', err) | |
| handleError() | |
| // Implement retry logic or more detailed error feedback | |
| if (err.name === 'NotAllowedError') { | |
| console.warn('Autoplay prevented by browser. User interaction required.') | |
| } else if (err.name === 'NotSupportedError') { | |
| console.error('Video format not supported by this browser') | |
| } | |
| }) | |
| // Update texture in animation loop | |
| mesh.onBeforeRender = () => { | |
| if (video.readyState === video.HAVE_ENOUGH_DATA) { | |
| if (material.map !== texture) { | |
| material.map = texture | |
| material.needsUpdate = true | |
| } | |
| texture.needsUpdate = true | |
| } | |
| } | |
| } |
✅ Addressed in commit 3c30cd3
|
Deployed to Vercel Preview: https://prismarine-akh6wqc7y-zaro.vercel.app |
|
@Phoenix616 I finished it. You can use it to load images in high-quality resolution (I'm still grateful you implemented map support some time ago as its nice to have compatible options for existing servers/solutions). By default, I restricted loading from other origins to avoid abuse like client IP leaking when source can be changed by someone else test: addTestVideo() |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
renderer/viewer/lib/worldrendererThree.ts (5)
743-855: Debug visualization should be conditionalThe
positionMeshExactmethod creates several debug meshes that are always added to the scene. In a production environment, these should be conditionally included based on a debug flag.positionMeshExact ( mesh: THREE.Mesh, rotation: number, startPosition: { x: number, y: number, z: number }, width: number, height: number, depth = 1, + debug = false ) { // ... existing positioning code ... // Create a group to hold our mesh and markers const debugGroup = new THREE.Group() debugGroup.add(mesh) + if (debug) { // Add a marker at the starting position (should be exactly at pos) const startMarker = new THREE.Mesh( new THREE.BoxGeometry(0.1, 0.1, 0.1), new THREE.MeshBasicMaterial({ color: 0xff_00_00 }) ) startMarker.position.copy(new THREE.Vector3(startPosition.x, startPosition.y, startPosition.z)) debugGroup.add(startMarker) // ... other debug markers and helpers ... + } return { mesh, debugGroup } }
675-685: Add error recovery for video playback operationsThe video control methods like
setVideoPlayingshould include better error handling to ensure robustness when controlling videos.setVideoPlaying (id: string, playing: boolean) { const videoData = this.customMedia.get(id) if (videoData?.video) { if (playing) { - videoData.video.play().catch(console.error) + videoData.video.play().catch(err => { + console.error(`Failed to play video ${id}:`, err) + // Implement retry logic or user notification + if (err.name === 'NotAllowedError') { + console.warn('Autoplay prevented by browser. User interaction required.') + // You might want to add a UI indicator that user interaction is needed + } + }) } else { videoData.video.pause() } } }
857-895: Remove or conditionally include test methods in production codeThe test methods
createTestCanvasTextureandaddTestMeshExactshould not be included in production code or should be conditionally excluded in production builds.Consider moving these methods to a separate debug/development module that's only included in development builds, or add a condition to disable them in production:
+ // Development/testing utilities - not used in production + #if DEBUG createTestCanvasTexture() { // ...existing code... } addTestMeshExact(rotationNum: number) { // ...existing code... } + #endif
686-706: Video controls methods should validate input parametersThe media control methods accept parameters without validation. Add range checks to ensure values are within expected bounds.
setVideoSeeking (id: string, seconds: number) { const videoData = this.customMedia.get(id) if (videoData?.video) { + // Ensure seconds is within valid range + if (seconds < 0) { + console.warn(`Invalid seek time: ${seconds}. Setting to 0.`) + seconds = 0 + } else if (videoData.video.duration && seconds > videoData.video.duration) { + console.warn(`Seek time exceeds video duration. Setting to end of video.`) + seconds = videoData.video.duration + } videoData.video.currentTime = seconds } } setVideoVolume (id: string, volume: number) { const videoData = this.customMedia.get(id) if (videoData?.video) { + // Clamp volume between 0 and 1 + volume = Math.max(0, Math.min(1, volume)) videoData.video.volume = volume } } setVideoSpeed (id: string, speed: number) { const videoData = this.customMedia.get(id) if (videoData?.video) { + // Ensure reasonable playback rate + if (speed <= 0) { + console.warn(`Invalid playback speed: ${speed}. Setting to 0.1.`) + speed = 0.1 + } else if (speed > 16) { + console.warn(`Excessive playback speed: ${speed}. Setting to 16.`) + speed = 16 + } videoData.video.playbackRate = speed } }
605-626: Consider alternative texture loading strategy for imagesThe current image loading implementation could be enhanced with a loading manager to handle multiple image loads and provide better error reporting.
+ // Add this to the class declaration + private textureLoader: THREE.TextureLoader; + + constructor(...) { + // Existing code... + this.textureLoader = new THREE.TextureLoader(); + + // Set up global error handling + this.textureLoader.manager.onError = (url) => { + console.error(`Failed to load texture: ${url}`); + }; + } // Then in the addMedia method: - const texture = video - ? new THREE.VideoTexture(video) - : new THREE.TextureLoader().load(props.src, () => { - if (this.customMedia.get(id)?.texture === texture) { - material.map = texture - material.needsUpdate = true - } - }, undefined, handleError) // todo cache + const texture = video + ? new THREE.VideoTexture(video) + : this.textureLoader.load( + props.src, + (loadedTexture) => { + if (this.customMedia.get(id)?.texture === texture) { + material.map = loadedTexture; + material.needsUpdate = true; + } + }, + undefined, + handleError + );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
renderer/viewer/lib/worldrendererThree.ts(3 hunks)
🔇 Additional comments (8)
renderer/viewer/lib/worldrendererThree.ts (8)
19-29: Well-designed MediaProperties interfaceThe interface is comprehensive and covers all the necessary properties for positioning, sizing, and configuring media elements. The optional parameters provide good flexibility for different use cases.
43-49: Good choice of Map structure for media trackingUsing a Map with string IDs as keys is appropriate for storing and retrieving media objects. The structure contains all necessary components (mesh, video element, texture, and UV mapping function).
479-502: Position calculation handles rotation effectivelyThe method correctly calculates positions based on rotation values, ensuring consistent positioning regardless of the rotation angle.
562-574: Security validation properly implementedGood practice to validate origin and destroy existing media with the same ID before creating a new one. The file type detection is simple but effective.
627-644: Video texture update optimization neededThe current implementation updates textures inside individual
onBeforeRendercallbacks, which could lead to performance issues with multiple videos. Consider consolidating texture updates into a single animation loop.- mesh.onBeforeRender = () => { - if (video.readyState === video.HAVE_ENOUGH_DATA) { - if (material.map !== texture) { - material.map = texture - material.needsUpdate = true - } - texture.needsUpdate = true - } - } // Instead, add a single update method to the class: + private updateAllVideoTextures() { + for (const [id, mediaData] of this.customMedia.entries()) { + if (mediaData.video && mediaData.video.readyState === mediaData.video.HAVE_ENOUGH_DATA) { + const mesh = mediaData.mesh.children[0] as THREE.Mesh; + const material = mesh.material as THREE.MeshBasicMaterial; + + if (material.map !== mediaData.texture) { + material.map = mediaData.texture; + material.needsUpdate = true; + } + mediaData.texture.needsUpdate = true; + } + } + } // And call this in your render method: + render() { + // Existing code... + this.updateAllVideoTextures(); + // Render scene... + }
708-729: Thorough resource cleanup in destroyMediaThe method properly cleans up all resources, including pausing and clearing the video source, removing meshes from the scene, and disposing of textures and geometries. This is essential to prevent memory leaks.
504-531: Good error handling with fallback texturesCreating error textures as fallbacks is a good practice, ensuring visual feedback when media fails to load.
646-664: UV mapping implementation is flexible and reusableThe UV mapping configuration is well-implemented, allowing for texture cropping and scaling. Good choice to make it a separate function that can be called after initialization.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
src/customChannels.ts (4)
154-161:⚠️ Potential issueFix field naming inconsistency in packet structure.
There's an inconsistency between the field names in the packet structure (prefixed with underscore) and their usage in the handler on line 199 (without underscore). For example,
_volume,_background, and_opacityare defined with underscores but accessed without them.- { name: '_volume', type: 'f32' }, // 0 + { name: 'volume', type: 'f32' }, // 0 - { name: '_aspectRatioMode', type: 'i16' }, // 0 + { name: 'aspectRatioMode', type: 'i16' }, // 0 - { name: '_background', type: 'i16' }, // 0 + { name: 'background', type: 'i16' }, // 0 - { name: '_opacity', type: 'i16' }, // 1 + { name: 'opacity', type: 'i16' }, // 1 - { name: '_cropXStart', type: 'f32' }, // 0 + { name: 'cropXStart', type: 'f32' }, // 0 - { name: '_cropYStart', type: 'f32' }, // 0 + { name: 'cropYStart', type: 'f32' }, // 0 - { name: '_cropXEnd', type: 'f32' }, // 0 + { name: 'cropXEnd', type: 'f32' }, // 0 - { name: '_cropYEnd', type: 'f32' }, // 0 + { name: 'cropYEnd', type: 'f32' }, // 0
198-226: 🛠️ Refactor suggestionAdd error handling for media operations.
The current implementation lacks error handling for potential issues like invalid URLs or playback failures. Media operations can fail due to various reasons including network issues, unsupported formats, or CORS restrictions.
bot._client.on(ADD_CHANNEL as any, (data) => { const { id, x, y, z, width, height, rotation, source, loop, background, opacity } = data const worldRenderer = viewer.world as WorldRendererThree // Destroy existing video if it exists worldRenderer.destroyMedia(id) + try { // Add new video worldRenderer.addMedia(id, { position: { x, y, z }, size: { width, height }, // side: 'towards', src: source, rotation: rotation as 0 | 1 | 2 | 3, doubleSide: false, background, opacity: opacity / 100, allowOrigins: options.remoteContentNotSameOrigin === false ? [getCurrentTopDomain()] : options.remoteContentNotSameOrigin }) // Set loop state if (!loop) { const videoData = worldRenderer.customMedia.get(id) if (videoData?.video) { videoData.video.loop = false } } + } catch (error) { + console.error(`Failed to add media ${id}:`, error) + } })
228-233: 🛠️ Refactor suggestionAdd error handling for media control operations.
Similar to the Add handler, these control operations lack error handling. Wrap the operations in try-catch blocks to gracefully handle failures.
// Example for one handler (apply similar pattern to all): bot._client.on(PLAY_CHANNEL as any, (data) => { const { id } = data const worldRenderer = viewer.world as WorldRendererThree + try { worldRenderer.setVideoPlaying(id, true) + } catch (error) { + console.error(`Failed to play video ${id}:`, error) + } })Also applies to: 235-240, 242-247, 249-254, 256-261, 263-268
329-361: 🛠️ Refactor suggestionAdd error handling to addTestVideo and address TODO comment.
The function contains a TODO comment about broken UV mapping that should be addressed or documented better. Also, it needs error handling for media loading failures.
const addTestVideo = (rotation = 0 as 0 | 1 | 2 | 3, scale = 1, isImage = false) => { + try { const block = window.cursorBlockRel() if (!block) return const { position: startPosition } = block const worldRenderer = viewer.world as WorldRendererThree // Add video with proper positioning worldRenderer.addMedia('test-video', { position: { x: startPosition.x, y: startPosition.y + 1, z: startPosition.z }, size: { width: scale, height: scale }, src: isImage ? 'https://bucket.mcraft.fun/test_image.png' : 'https://bucket.mcraft.fun/test_video.mp4', rotation, // doubleSide: true, background: 0x00_00_00, // Black color // TODO broken // uvMapping: { // startU: 0, // endU: 1, // startV: 0, // endV: 1 // }, opacity: 1, allowOrigins: true, }) + } catch (error) { + console.error('Error in addTestVideo:', error) + } }Also, consider creating a GitHub issue to track the broken UV mapping functionality mentioned in the TODO comment.
Would you like me to create an issue to track fixing the UV mapping functionality?
🧹 Nitpick comments (3)
src/customChannels.ts (3)
216-216: Validate and document CORS configuration options.The code uses
options.remoteContentNotSameOriginfor CORS configuration, but its purpose and valid values aren't clearly documented in the code. Consider adding validation and documentation.- allowOrigins: options.remoteContentNotSameOrigin === false ? [getCurrentTopDomain()] : options.remoteContentNotSameOrigin + // allowOrigins config: + // - false: Only allow same domain (more secure) + // - true: Allow all origins (less secure) + // - Array of strings: Allow specific domains + allowOrigins: options.remoteContentNotSameOrigin === false + ? [getCurrentTopDomain()] + : (Array.isArray(options.remoteContentNotSameOrigin) || options.remoteContentNotSameOrigin === true + ? options.remoteContentNotSameOrigin + : [getCurrentTopDomain()]),
306-307: Use TypeScript type guards to ensure safe property access.The code assumes that
videoData.mesh.children[0]is aTHREE.Mesh, but this should be verified with a type guard to prevent runtime errors.// Check intersection with all video meshes for (const [id, videoData] of worldRenderer.customMedia.entries()) { // Get the actual mesh (first child of the group) - const mesh = videoData.mesh.children[0] as THREE.Mesh - if (!mesh) continue + if (!videoData.mesh?.children?.length) continue + const child = videoData.mesh.children[0] + if (!(child instanceof THREE.Mesh)) continue + const mesh = child
205-215: Improve validation of media properties.The code doesn't validate important properties like position, size, and source URL. Adding validation would prevent potential rendering issues or crashes.
// Destroy existing video if it exists worldRenderer.destroyMedia(id) try { + // Validate required parameters + if (!source) { + throw new Error('Media source URL is required') + } + + if (typeof x !== 'number' || typeof y !== 'number' || typeof z !== 'number') { + throw new Error('Invalid position coordinates') + } + + if (typeof width !== 'number' || width <= 0 || typeof height !== 'number' || height <= 0) { + throw new Error('Invalid size dimensions') + } + // Add new video worldRenderer.addMedia(id, { position: { x, y, z }, size: { width, height },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/customChannels.ts(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-and-deploy
🔇 Additional comments (1)
src/customChannels.ts (1)
364-383:✅ Verification successful
Enhance domain detection logic and add unit tests.
The
getCurrentTopDomainfunction handles some special cases for domain detection, but might not cover all possibilities (like .co.jp, .org.uk, etc.). Consider using an established library or expanding the logic.function getCurrentTopDomain (): string { const { hostname } = location // Split hostname into parts const parts = hostname.split('.') // Handle special cases like co.uk, com.br, etc. if (parts.length > 2) { // Check for common country codes with additional segments + // Known TLDs that commonly have subdomains + const knownTLDs = ['co', 'com', 'org', 'gov', 'edu', 'ac', 'net'] - if (parts.at(-2) === 'co' || - parts.at(-2) === 'com' || - parts.at(-2) === 'org' || - parts.at(-2) === 'gov') { + if (knownTLDs.includes(parts.at(-2) || '')) { // Return last 3 parts (e.g., example.co.uk) return parts.slice(-3).join('.') } } // Return last 2 parts (e.g., example.com) return parts.slice(-2).join('.') }Let's check if this function works correctly for various domains:
🏁 Script executed:
#!/bin/bash # Test the getCurrentTopDomain function with various domains cat << 'EOF' > test-domains.js const domains = [ 'example.com', 'sub.example.com', 'example.co.uk', 'sub.example.co.uk', 'example.com.br', 'example.org.uk', 'example.net.au', 'localhost' ]; // Simulated implementation function getCurrentTopDomain(hostname) { const parts = hostname.split('.'); if (parts.length > 2) { const knownTLDs = ['co', 'com', 'org', 'gov', 'edu', 'ac', 'net']; if (knownTLDs.includes(parts.at(-2))) { return parts.slice(-3).join('.'); } } return parts.slice(-2).join('.'); } // Test each domain console.log('Domain Tests:'); domains.forEach(domain => { console.log(`${domain} -> ${getCurrentTopDomain(domain)}`); }); EOF node test-domains.jsLength of output: 288
The enhanced domain detection logic and unit tests pass all the current test cases.
- The new
knownTLDslist effectively detects domains likeexample.co.uk,example.com.br,example.org.uk, and more.- The unit tests confirm that the function returns the expected top-level domain for each tested input.
- While this enhancement improves coverage, consider future enhancements if additional edge cases (e.g., domains like
.co.jp) need to be supported.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/customChannels.ts (1)
16-17: Consider renaming for clarity.
Having two register functions with inconsistent naming (registerMediaChannelsvs.registeredJeiChannel) can be confusing. RenamingregisteredJeiChanneltoregisterJeiChannelor similar might improve consistency.renderer/viewer/lib/worldrendererThree.ts (2)
19-29: Interface looks well-defined.
MediaPropertiescontains the fields needed for both images and video. Consider adding doc comments if you want more clarity on each field.
479-506: Potential refactor to unify texture creation.
createErrorTextureandcreateBackgroundTextureshare some logic (canvas creation, fill style). A shared helper method might reduce duplication.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
renderer/viewer/lib/worldrendererThree.ts(3 hunks)src/customChannels.ts(4 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
src/customChannels.ts (4)
src/mineflayer/plugins/mouse.ts (1) (1)
bot(143-150)renderer/viewer/lib/worldrendererThree.ts (1) (1)
WorldRendererThree(31-871)src/optionsStorage.ts (1) (1)
options(187-192)renderer/viewer/lib/viewer.ts (2) (2)
camera(32-34)camera(36-38)
🔇 Additional comments (22)
src/customChannels.ts (10)
2-4: Imports look valid and necessary.
No issues found—these imports are indeed used later in the file.
6-6: Import for JEI custom categories is valid.
This is used later for managing JEI category entries.
97-97: Packet field naming mismatch.
The packet structure defines_categoryTitle, but the handler destructurescategoryTitle.
111-115: Logic for empty item list looks correct.
Removing the category ifitems === ''is straightforward and appears to fulfill the intended behavior.
139-290: Add error handling for media operations.
Although this code successfully registers and handles media channels, there is no try-catch or fallback for failures when adding or controlling media (e.g., invalid URL, network errors, or unsupported formats).
292-292: Channel constant naming is consistent.
DeclaringMEDIA_INTERACTION_CHANNELclearly indicates its use case.
294-296: Simple and effective helper for sending video interactions.
No issues found with sending the interaction data downstream.
298-330: Intersection logic is clear and straightforward.
This function cleanly identifies the hovered video’s UV coordinates. The loop-based approach is fine for a modest number of videos.
333-365: Include error handling and address the TODO note.
Like previous suggestions, error handling is missing for the test video function. Also, the// TODO brokencomment on UV mapping remains unresolved.
368-387: Domain extraction logic appears correct.
Returns the top-level domain for potential CORS handling.renderer/viewer/lib/worldrendererThree.ts (12)
43-48: Data structure for media is solid and flexible.
Storing each media’s mesh, video element, and texture in a map is straightforward to manage.
508-528: Background texture creation is concise.
This snippet is clear with no apparent issues.
537-619: Performance hint: multiple onBeforeRender watchers might degrade performance.
This approach works for a small number of videos. However, repeated calls in each frame for many videos might be less efficient than a single playback loop.
651-660: Setting video playback state is straightforward.
No issues found with toggling play/pause.
662-667: Seeking logic is correct.
Directly settingcurrentTimeis a valid approach.
669-674: Volume control is implemented properly.
No issues here.
676-681: Playback speed control is simple and effective.
No issues here.
683-704: Destroying media is thorough.
Pausing the video, clearing its src, disposing textures and materials, and removing from the scene helps avoid leaks.
718-731: Positioning logic is correct.
No immediate concerns with orientation adjustments for the plane geometry.
832-842: Test canvas texture is fine.
Simple utility for debug or demonstration.
844-870: Demonstration mesh method looks fine.
No issues for a dev/test function.
530-535:❓ Verification inconclusive
Verify domain check logic for security.
UsingendsWith(origin)may allow partial matches (e.g.,evilmydomain.commatchingmydomain.com). If strict subdomain matching is intended, confirm or enhance validation.
🏁 Script executed:
#!/bin/bash # Checking usage of validateOrigin to see how 'endsWith' is used: rg -A 5 "validateOrigin"Length of output: 1149
Action Required: Confirm and Update Domain Validation Logic
The
validateOrigin()method currently usesurl.origin.endsWith(origin), which might inadvertently allow partial matches (for example,"evilmydomain.com"would match"mydomain.com"). Please verify if:
- The intended behavior is to allow subdomain matching.
- A stricter check (such as an exact comparison or verifying that the allowed origin is preceded by a dot for subdomains) is required for enhanced security.
If a stricter verification is desired, consider updating the logic. For instance, one alternative might be:
return allowOrigins.some(origin => url.origin === origin || url.origin.endsWith(`.${origin}`) )This change ensures either a full match or that the origin is a proper subdomain of the allowed origin.
- Location:
renderer/viewer/lib/worldrendererThree.ts(Lines 530-535)
PR Type
Enhancement
Description
Introduced video rendering capabilities in the
WorldRendererThreeclass.Registered custom media channels for handling video operations.
Added a test utility for adding videos to the scene.
Changes walkthrough 📝
worldrendererThree.ts
Introduced video rendering capabilities in WorldRendererThreerenderer/viewer/lib/worldrendererThree.ts
VideoPropertiesinterface for video configurations.videos.
customVideosmap to manage video elements and textures.customChannels.ts
Added media channel registration and test utilitysrc/customChannels.ts
destroy.
WorldRendererThree.Summary by CodeRabbit