Plan2D #247
Plan2D #247
Conversation
WalkthroughThe pull request introduces several updates across multiple files in the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Outside diff range and nitpick comments (8)
src/vim-loader/progressive/instancedSubmesh.ts (1)
11-11: LGTM! Consider adding a brief comment for clarity.The addition of the
readonly merged = falseproperty is a good refactoring choice. It simplifies the code while maintaining the same behavior as the previous getter method.Consider adding a brief comment to explain the purpose of this property, for example:
/** Indicates whether this submesh is merged. Always false for InstancedSubmesh. */ readonly merged = falseThis would help future developers understand the intent and invariant nature of this property.
src/vim-webgl-viewer/gizmos/gizmos.ts (2)
73-73: LGTM: Plans2D initialization in constructorThe
Plans2Dinstance is correctly initialized in the constructor.Consider adding a brief comment explaining the purpose of
Plans2Dfor better code documentation, similar to other properties in this class. For example:/** * The 2D plans gizmo for [brief explanation of its purpose]. */ readonly plans: Plans2D
Line range hint
82-92: Update dispose method to include new Plans2D instanceThe
disposemethod should be updated to include cleanup for the newplansproperty. This ensures proper resource management when theGizmosinstance is no longer needed.Consider updating the
disposemethod as follows:dispose () { this.viewer.viewport.canvas.parentElement?.removeChild(this.axes.canvas) this._measure.clear() this.section.dispose() this.loading.dispose() this.orbit.dispose() this.rectangle.dispose() this.axes.dispose() this.plans.dispose() // Add this line }Note: This assumes that
Plans2Dhas adisposemethod. If it doesn't, you may need to implement one or handle cleanup differently based on thePlans2Dimplementation.src/vim-loader/colorAttributes.ts (1)
104-106: LGTM: Special handling forSimpleMeshadded, but documentation needed.The addition of special handling for
SimpleMeshinstances is consistent with the changes described in the summary. However, it would be beneficial to add a comment explaining whySimpleMeshinstances are treated differently in this context.Consider adding a comment above the new conditional block to explain the rationale behind this special case, such as:
// SimpleMesh instances do not require color resetting as they handle colors differently if (sub instanceof SimpleMesh) { return }src/vim-webgl-viewer/gizmos/plans2D.ts (1)
82-82: SetcrossOriginconditionally based on the image sourceSetting
image.crossOrigin = 'anonymous'may cause issues if the image is served from the same origin and doesn't require CORS handling. Consider setting it conditionally or providing guidance on when it should be set.Apply this diff:
- image.crossOrigin = 'anonymous' // Use this if loading images from a different origin + // Set crossOrigin only if loading images from a different origin + if (shouldUseCrossOrigin(imageUrl)) { + image.crossOrigin = 'anonymous' + }And define the
shouldUseCrossOriginfunction based on your application's logic.src/vim-webgl-viewer/gizmos/plan2D.ts (2)
230-234: Avoid using the non-null assertion operator to handle potential null values.In the
getBoundingBox()method, using the non-null assertion operator!assumes thatboundingBoxis always defined after computation. It's safer to handle possiblenullorundefinedvalues explicitly to prevent potential runtime errors.Modify the return statement to handle undefined
boundingBox:return this.mesh.geometry.boundingBox! + return this.mesh.geometry.boundingBox || new THREE.Box3()
98-100: Consider making the initial gizmo mode configurable.The gizmo is initially set to 'translate' mode. Depending on user requirements, it might be beneficial to allow the initial mode to be configurable or align with user preferences.
Provide an option to set the initial gizmo mode:
constructor (viewer: Viewer, canvas: HTMLCanvasElement, initialMode: 'translate' | 'rotate' | 'scale' = 'translate') { // ... existing code ... // Enable interaction modes - gizmo.setMode('translate') // Can be 'translate', 'rotate', or 'scale' + gizmo.setMode(initialMode)This change allows the initial gizmo mode to be specified when creating an instance of
Plan2D.src/vim-webgl-viewer/camera/camera.ts (1)
118-121: Combine sentences in the comment for clarityThe comment for
freezecan be made clearer by combining the sentences into one.Apply this diff to improve the comment:
/** - * Immediately stops the camera movement. And prevents any further movement until set to false. + * Immediately stops the camera movement and prevents any further movement until set to false. */ freeze: boolean
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (13)
- package.json (1 hunks)
- src/main.ts (2 hunks)
- src/vim-loader/colorAttributes.ts (2 hunks)
- src/vim-loader/mesh.ts (4 hunks)
- src/vim-loader/objectAttributes.ts (2 hunks)
- src/vim-loader/progressive/insertableSubmesh.ts (1 hunks)
- src/vim-loader/progressive/instancedSubmesh.ts (1 hunks)
- src/vim-webgl-viewer/camera/camera.ts (4 hunks)
- src/vim-webgl-viewer/gizmos/gizmos.ts (3 hunks)
- src/vim-webgl-viewer/gizmos/plan2D.ts (1 hunks)
- src/vim-webgl-viewer/gizmos/plans2D.ts (1 hunks)
- src/vim-webgl-viewer/raycaster.ts (2 hunks)
- src/vim-webgl-viewer/selection.ts (1 hunks)
🧰 Additional context used
🔇 Additional comments (23)
src/vim-loader/progressive/insertableSubmesh.ts (1)
12-12: Approve the addition of themergedproperty with suggestions.The addition of the
readonly merged = trueproperty looks good. However, I have a few suggestions to ensure its effectiveness and maintainability:
Consider adding a comment to document the purpose of this property. It would help other developers understand why it's always true and how it's used.
Verify that all usages of the previous
mergedgetter have been updated to use this property instead. This change from a getter to a property might affect how the class is used elsewhere in the codebase.Given that this property is always
true, consider if it's being used for type checking or interface implementation. If not, it might be more appropriate to use a type assertion or implement an interface explicitly, rather than having a constant property.To help verify the usage and impact of this change, you can run the following script:
This script will help identify where the
mergedproperty is being used and if there are any interfaces that might require it.✅ Verification successful
No usages of
mergedfound in the codebase.The
readonly merged = trueproperty has been successfully added toInsertableSubmesh. No references or dependencies on this property were detected in the current codebase, indicating that its addition does not impact existing functionality.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for usages of InsertableSubmesh.merged in the codebase # Search for usages of InsertableSubmesh.merged echo "Usages of InsertableSubmesh.merged:" rg --type typescript "InsertableSubmesh.*\.merged" # Search for potential interface definitions that might require this property echo "\nPotential interfaces requiring 'merged' property:" rg --type typescript "interface.*\{[^}]*merged\s*:[^}]*\}"Length of output: 383
Script:
#!/bin/bash # Description: Check for usages of InsertableSubmesh.merged in TypeScript files # Search for usages of InsertableSubmesh.merged echo "Usages of InsertableSubmesh.merged:" rg -g "*.ts" -g "*.tsx" "InsertableSubmesh\..*\.merged" # Search for potential interface definitions that might require this property echo "\nPotential interfaces requiring 'merged' property:" rg -g "*.ts" -g "*.tsx" "interface\s+\w+\s*\{[^}]*merged\s*:\s*[^}]*\}"Length of output: 331
package.json (1)
56-56: New dependency added: pdfjs-distThe addition of
pdfjs-distas a dependency is noted. This likely introduces PDF-related functionality to the project.Please ensure the following:
Verify that PDF functionality aligns with the project scope and requirements.
Check for any potential conflicts with existing dependencies.
Review known vulnerabilities for
[email protected]:Update project documentation to reflect this new capability.
Would you like assistance in updating the project documentation or creating a GitHub issue to track these tasks?
src/main.ts (2)
1-2: LGTM: Import statement change for THREE.jsThe change to import the entire THREE.js module as a namespace is a good practice. It provides more flexibility and is consistent with common usage patterns of the THREE.js library.
Line range hint
1-43: Overall changes look good, consider updating documentationThe changes to the
loadfunction enhance the viewer's capabilities by adding support for plans. The modifications integrate well with the existing code structure and are consistent with the PR objectives.To ensure maintainability and ease of use:
- Consider updating any relevant documentation to reflect these new capabilities.
- If there are unit tests for the
loadfunction, they should be updated to cover the new functionality.To ensure that documentation is up-to-date, let's check for any README files or documentation in the project:
This will help identify any documentation that might need to be updated to reflect the new plan functionality.
src/vim-webgl-viewer/gizmos/gizmos.ts (2)
10-10: LGTM: New import statement for Plans2DThe import statement for
Plans2Dis correctly formatted and necessary for the new property being added to theGizmosclass.
57-58: LGTM: New Plans2D propertyThe
plansproperty is correctly declared as readonly and of typePlans2D. Its public accessibility and placement within the class are consistent with the existing structure.src/vim-loader/objectAttributes.ts (3)
6-6: LGTM: Import statement updated correctly.The addition of
SimpleMeshto the import statement is consistent with its usage in theAttributeTargettype definition. This change enhances the module's capability to work with different mesh types.
54-54: LGTM: apply method updated for flexibility.The removal of the type assertion for
subasMergedSubmeshallows for more generic handling of thesubvariable, which is consistent with the expandedAttributeTargettype. This change improves the flexibility of theObjectAttributeclass in dealing with different types of mesh attributes.To ensure that this change doesn't introduce any compatibility issues, please verify that the
applyMergedmethod can handle all possible types defined inAttributeTarget. Run the following script to check for potential issues:#!/bin/bash # Description: Check compatibility of applyMerged method with AttributeTarget types # Test 1: Check the applyMerged method implementation echo "Implementation of applyMerged method:" ast-grep --lang typescript --pattern $'class ObjectAttribute { $$$ private applyMerged(sub: $_, number: number) { $$$ } $$$ }' # Test 2: Look for any type checks or assertions within applyMerged echo "Type checks or assertions in applyMerged:" rg --type typescript "applyMerged.*\{" -A 10 | rg "(instanceof|as |typeof)" # Test 3: Check for usage of properties specific to MergedSubmesh echo "Usage of MergedSubmesh-specific properties:" rg --type typescript "applyMerged.*\{" -A 20 | rg "(meshStart|meshEnd)"
8-8: LGTM: AttributeTarget type updated correctly.The addition of
SimpleMeshto theAttributeTargettype definition expands the possible types that can be used for mesh attributes, which is consistent with the updated import statement.To ensure that this change doesn't introduce any unintended side effects, please verify the behavior of methods that use the
AttributeTargettype, particularly theapplymethod. Run the following script to check for any potential issues:src/vim-loader/colorAttributes.ts (2)
6-6: LGTM: Import ofSimpleMeshadded correctly.The addition of
SimpleMeshto the import statement is consistent with the changes described in the summary and follows the existing import style.
Line range hint
1-193: Consider reviewing the entireColorAttributeclass for consistentSimpleMeshhandling.With the introduction of special handling for
SimpleMeshinstances in theresetMergedColormethod, it's important to ensure that other methods in theColorAttributeclass handleSimpleMeshinstances consistently. This may include methods likeapplyMergedColor,applyInstancedColor, and others.To assist with this review, you can run the following script to identify other methods in the
ColorAttributeclass that might need updates:Please review the output of this script and consider whether any other methods need to be updated to handle
SimpleMeshinstances consistently.src/vim-webgl-viewer/selection.ts (2)
10-10: LGTM: Import statement for Plan2D is correct.The import statement for
Plan2Dis correctly added and is necessary for the subsequent type definition change.
Line range hint
1-248: Summary: Plan2D integration looks good. Ensure comprehensive testing.The changes to include
Plan2Das a selectable object type have been implemented correctly. TheSelectionclass implementation appears to handle objects generically, which should accommodate the newPlan2Dtype without further modifications.To ensure the integration is complete and correct:
- Verify that all
Selectionclass methods work as expected withPlan2Dobjects.- Add unit tests specifically for selecting, deselecting, and manipulating
Plan2Dobjects.- Update any relevant documentation to reflect the new selectable object type.
Run the following script to identify potential areas that might need additional testing:
This script will help identify any type-specific handling in the
Selectionclass methods, which might need to be updated or tested forPlan2Dobjects.src/vim-loader/mesh.ts (4)
129-129: Ensure correct handling of 'SealedSubmesh' in 'getSubmeshFromFace'The method
getSubmeshFromFacenow returnsSealedSubmesh. Verify that any code that calls this method and expects aStandardSubmeshis updated to handleSealedSubmesh.
137-137: Update code to handle 'SealedSubmesh' array from 'getSubmeshes'The
getSubmeshesmethod now returns an array ofSealedSubmeshinstances. Ensure that all usages of this method are compatible withSealedSubmeshand that any type assertions or expectations are updated accordingly.
167-168: Review changes to 'MergedSubmesh' and 'Submesh' typesThe
MergedSubmeshtype now includesSealedSubmesh,InsertableSubmesh, andSimpleMesh. TheSubmeshtype includesMergedSubmeshandInstancedSubmesh. Ensure that all code that uses these types is updated to handle the new type structure.Consider searching for usages of these types:
#!/bin/bash # Description: Find all usages of `MergedSubmesh` and `Submesh` to verify compatibility. # Test: Search for usages of `MergedSubmesh` and `Submesh` in TypeScript files. rg 'MergedSubmesh|Submesh' --type ts
118-118: Verify compatibility of 'SealedSubmesh' with 'StandardSubmesh'The method
getSubMeshnow returnsSealedSubmeshinstead ofStandardSubmesh. Ensure thatSealedSubmeshprovides all the methods and properties expected by code that usesgetSubMesh.You can run the following script to find all usages of
getSubMeshand check for any type incompatibilities:src/vim-webgl-viewer/raycaster.ts (4)
14-14: ImportingPlan2DThe
Plan2Dclass is now imported, enabling the raycasting functionality to includePlan2Dobjects.
50-59: Enhance raycasting to detectPlan2DobjectsThe
GetFirstHitmethod now includes logic to detectPlan2Dinstances in raycasting results. This broadens the interactable objects within the scene.
45-47: Ensure consistent usage ofGetFirstHitThe method
GetFirstVimHithas been replaced withGetFirstHit. Verify that all references toGetFirstVimHithave been updated throughout the codebase to prevent any broken references.Run the following script to check for any remaining references to
GetFirstVimHit:#!/bin/bash # Description: Search for any occurrences of 'GetFirstVimHit' that may have been missed. rg 'GetFirstVimHit'
30-30: Updateobjectproperty to includePlan2DThe
objectproperty inRaycastResultnow includesPlan2Das a possible type. Ensure that all usages ofRaycastResult.objecthandlePlan2Dinstances correctly to prevent any runtime errors.Run the following script to find all references to
RaycastResult.objectand check for compatibility withPlan2D:src/vim-webgl-viewer/camera/camera.ts (2)
160-160: LGTM!The private field
_freezeis appropriately initialized.
213-220: Verify if events should be dispatched whenfreezeis modifiedConsider whether the
freezeproperty change should trigger any events, such as_onValueChangedor_onMoved, to notify other components of the state change.
| const plan = await viewer.gizmos.plans.addPlan('https://vimdevelopment01storage.blob.core.windows.net/samples/floor_plan.png') | ||
| const plan2 = await viewer.gizmos.plans.addPlan('https://vimdevelopment01storage.blob.core.windows.net/samples/floor_plan.pdf') | ||
| plan.color = new THREE.Color(0x00ff00) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider parameterizing plan URLs and colors
The addition of plans to the viewer's gizmos enhances its capabilities. However, there are a few points to consider:
- The URLs for the plans are hardcoded. Consider parameterizing these for better maintainability.
- Only the first plan's color is set. Is this intentional? If not, consider setting colors for both plans or using a consistent approach.
Here's a suggestion to improve this section:
const planUrls = [
'https://vimdevelopment01storage.blob.core.windows.net/samples/floor_plan.png',
'https://vimdevelopment01storage.blob.core.windows.net/samples/floor_plan.pdf'
];
const planColors = [
new THREE.Color(0x00ff00),
new THREE.Color(0x0000ff) // Example second color
];
for (let i = 0; i < planUrls.length; i++) {
const plan = await viewer.gizmos.plans.addPlan(planUrls[i]);
plan.color = planColors[i];
}This approach allows for easier management of multiple plans and their colors.
| import { Plan2D } from './gizmos/plan2D' | ||
|
|
||
| export type SelectableObject = Object3D | GizmoMarker | ||
| export type SelectableObject = Object3D | GizmoMarker | Plan2D |
There was a problem hiding this comment.
💡 Codebase verification
Potential Issue: Selection class may not handle Plan2D objects
The SelectableObject type has been updated to include Plan2D, but there are no corresponding implementations in the Selection class methods to handle Plan2D objects. This omission could lead to unexpected behavior or runtime errors when Plan2D objects are selected.
src/vim-webgl-viewer/selection.ts- Line 12:
export type SelectableObject = Object3D | GizmoMarker | Plan2D
- Line 12:
🔗 Analysis chain
LGTM: SelectableObject type updated correctly. Verify class implementation.
The SelectableObject type has been correctly updated to include Plan2D. This change expands the selection capabilities as intended.
To ensure the Selection class correctly handles the new Plan2D type, please run the following verification script:
This script will help identify any potential issues or missing implementations related to the Plan2D type in the Selection class methods.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the Selection class methods handle Plan2D objects correctly.
# Test: Check if Plan2D is used in the Selection class methods
rg --type typescript -g 'src/vim-webgl-viewer/selection.ts' -e 'Plan2D' -C 5
# Test: Verify that no type errors are introduced by this change
ast-grep --lang typescript --pattern 'class Selection {
$$$
$_($$_: $$$Plan2D$$$) {
$$$
}
$$$
}'
Length of output: 484
Script:
#!/bin/bash
# Description: Verify that the Selection class methods handle Plan2D objects correctly.
# Test 1: Check if Plan2D is used in the Selection class methods
rg --type typescript -g 'src/vim-webgl-viewer/selection.ts' -e 'Plan2D' -C 5
# Test 2: Verify that no type errors are introduced by this change
ast-grep --lang typescript --pattern '
class Selection {
$$$
$_($$_: Plan2D) {
$$$
}
$$$
}
'
Length of output: 216
Script:
#!/bin/bash
# Description: Verify that the Selection class methods handle Plan2D objects correctly.
# Test 1: Check if Plan2D is used in the Selection class methods
rg -g 'src/vim-webgl-viewer/selection.ts' -e 'Plan2D' -C 5
# Test 2: Verify that no type errors are introduced by this change
ast-grep --lang typescript --pattern 'class Selection {
$$$
$_($$_: Plan2D) {
$$$
}
$$$
}'
Length of output: 1073
| async addPlan (url: string): Promise<Plan2D> { | ||
| let canvas: HTMLCanvasElement | ||
|
|
||
| // Determine the file type based on the file extension | ||
| const extension = url.split('.').pop()?.toLowerCase() | ||
| if (extension === 'pdf') { | ||
| canvas = await loadPdfAsImage(url) | ||
| } else if (['png', 'jpg', 'jpeg', 'gif', 'bmp'].includes(extension!)) { | ||
| canvas = await loadImageAsCanvas(url) | ||
| } else { | ||
| throw new Error('Unsupported file type') | ||
| } | ||
|
|
||
| const plan = new Plan2D(this._viewer, canvas) | ||
| this._viewer.renderer.add(plan.mesh) | ||
| return plan | ||
| } |
There was a problem hiding this comment.
Validate input URLs to enhance security
Loading resources from external URLs without validation can introduce security vulnerabilities such as Cross-Site Scripting (XSS) or injection attacks. Consider validating the input URL to ensure it conforms to expected formats or domains, or sanitize it before usage.
Apply this diff to add basic URL validation:
async addPlan (url: string): Promise<Plan2D> {
+ if (!isValidUrl(url)) {
+ throw new Error('Invalid URL')
+ }
let canvas: HTMLCanvasElement
// Determine the file type based on the file extensionAnd define the isValidUrl function:
function isValidUrl(url: string): boolean {
try {
new URL(url)
return true
} catch (e) {
return false
}
}Further enhancements could include checking against a whitelist of allowed domains.
| import * as PDF from 'pdfjs-dist' | ||
| import { Plan2D } from './plan2D' | ||
|
|
||
| PDF.GlobalWorkerOptions.workerSrc = 'node_modules/pdfjs-dist/build/pdf.worker.mjs' |
There was a problem hiding this comment.
Make the PDF worker source path dynamic
Setting a hardcoded path for PDF.GlobalWorkerOptions.workerSrc may lead to issues in different deployment environments where the path to pdf.worker.mjs might vary. Consider using a dynamic import or configuring the worker source path based on the environment to ensure compatibility.
Apply this diff to use a dynamic import:
-PDF.GlobalWorkerOptions.workerSrc = 'node_modules/pdfjs-dist/build/pdf.worker.mjs'
+import pdfjsWorker from 'pdfjs-dist/build/pdf.worker.entry'
+PDF.GlobalWorkerOptions.workerSrc = pdfjsWorkerEnsure that your build system supports loading worker files correctly.
📝 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.
| PDF.GlobalWorkerOptions.workerSrc = 'node_modules/pdfjs-dist/build/pdf.worker.mjs' | |
| import pdfjsWorker from 'pdfjs-dist/build/pdf.worker.entry' | |
| PDF.GlobalWorkerOptions.workerSrc = pdfjsWorker |
| const canvas = document.createElement('canvas') | ||
| canvas.width = image.width | ||
| canvas.height = image.height | ||
| const context = canvas.getContext('2d')! |
There was a problem hiding this comment.
Handle potential null return from getContext('2d')
Similar to the previous comment, ensure that the canvas.getContext('2d') call handles the possibility of returning null.
Apply this diff:
-const context = canvas.getContext('2d')!
+const context = canvas.getContext('2d')
+if (!context) {
+ reject(new Error('2D rendering context is not available'))
+ return
+}📝 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.
| const context = canvas.getContext('2d')! | |
| const context = canvas.getContext('2d') | |
| if (!context) { | |
| reject(new Error('2D rendering context is not available')) | |
| return | |
| } |
| // Create a plane geometry (width and height based on canvas dimensions) | ||
| this._geometry = new THREE.PlaneGeometry(100, 100) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Initialize plane geometry dimensions based on canvas size instead of scaling the mesh.
Currently, you're creating a PlaneGeometry with fixed dimensions and then scaling the mesh to match the canvas size. This can be simplified by directly initializing the geometry with the canvas dimensions, improving code clarity and potentially performance.
Apply this diff to modify the geometry initialization and remove the scaling:
// Create a plane geometry with dimensions based on canvas size
- this._geometry = new THREE.PlaneGeometry(100, 100)
+ const width = canvas.width * 0.001
+ const height = canvas.height * 0.001
+ this._geometry = new THREE.PlaneGeometry(width, height)
- frameMesh.scale.set(canvas.width, canvas.height, 1).multiplyScalar(0.001)Also applies to: 50-50
| window.addEventListener('keydown', onKey) | ||
| this._subs.push(() => window.removeEventListener('keydown', onKey)) |
There was a problem hiding this comment.
Avoid attaching global 'keydown' event listener to the window object and prevent default browser actions.
Attaching a 'keydown' event listener to the global window object can lead to unintended side effects and potential conflicts with other components. Additionally, handling keys like 't', 'r', and 's' without preventing default browser behavior might interfere with the application's functionality.
Solution: Attach the event listener to the viewer's canvas and prevent default actions.
Apply this diff to attach the event listener to the canvas and prevent default behavior:
const onKey = (event: KeyboardEvent): void => {
if (!this.selected) return
+ event.preventDefault()
switch (event.key) {
case 't': // 't' for translate
gizmo.setMode('translate')
break
case 'r': // 'r' for rotate
gizmo.setMode('rotate')
break
case 's': // 's' for scale
gizmo.setMode('scale')
break
}
}
- window.addEventListener('keydown', onKey)
- this._subs.push(() => window.removeEventListener('keydown', onKey))
+ this._viewer.viewport.canvas.tabIndex = 0 // Ensure the canvas can receive focus
+ this._viewer.viewport.canvas.addEventListener('keydown', onKey)
+ this._subs.push(() => this._viewer.viewport.canvas.removeEventListener('keydown', onKey))Additional Note: Setting tabIndex = 0 on the canvas element allows it to receive keyboard events when focused. Ensure that the canvas is focused appropriately within your application.
Also applies to: 103-115
| get size (): THREE.Vector2 { | ||
| return new THREE.Vector2(this.mesh.scale.x, this.mesh.scale.y) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize 'size' getter and setter to reuse a Vector2 instance and improve performance.
Creating a new THREE.Vector2 instance every time the size getter is called can lead to unnecessary memory allocations. Reusing a single instance improves performance, especially if size is accessed frequently.
Introduce a private _size property and update it accordingly:
+ private readonly _size: THREE.Vector2 = new THREE.Vector2()
get size (): THREE.Vector2 {
- return new THREE.Vector2(this.mesh.scale.x, this.mesh.scale.y)
+ return this._size.set(this.mesh.scale.x, this.mesh.scale.y)
}
set size (value: THREE.Vector2) {
this.mesh.scale.set(value.x, value.y, 1)
this.mesh.geometry.computeBoundingBox()
this._viewer.renderer.needsUpdate = true
+ this._size.copy(value)
}Also applies to: 220-224
| return this._force | ||
| ? new THREE.Vector3(1, 1, 1) | ||
| : this._freeze | ||
| ? new THREE.Vector3(0, 0, 0) | ||
| : this._allowedMovement |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve readability by replacing nested ternary operators with if-else statements
Nested ternary operators can be hard to read and maintain. Consider refactoring the getter to use if-else statements for better clarity.
Apply this diff to improve readability:
get allowedMovement () {
- return this._force
- ? new THREE.Vector3(1, 1, 1)
- : this._freeze
- ? new THREE.Vector3(0, 0, 0)
- : this._allowedMovement
+ if (this._force) {
+ return new THREE.Vector3(1, 1, 1)
+ } else if (this._freeze) {
+ return new THREE.Vector3(0, 0, 0)
+ } else {
+ return this._allowedMovement
+ }
}📝 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.
| return this._force | |
| ? new THREE.Vector3(1, 1, 1) | |
| : this._freeze | |
| ? new THREE.Vector3(0, 0, 0) | |
| : this._allowedMovement | |
| get allowedMovement () { | |
| if (this._force) { | |
| return new THREE.Vector3(1, 1, 1) | |
| } else if (this._freeze) { | |
| return new THREE.Vector3(0, 0, 0) | |
| } else { | |
| return this._allowedMovement | |
| } | |
| } |
| return this._force | ||
| ? new THREE.Vector2(1, 1) | ||
| : this._freeze | ||
| ? new THREE.Vector2(0, 0) | ||
| : this._allowedRotation |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve readability by replacing nested ternary operators with if-else statements
Nested ternary operators can be hard to read and maintain. Consider refactoring the getter to use if-else statements for better clarity.
Apply this diff to improve readability:
get allowedRotation () {
- return this._force
- ? new THREE.Vector2(1, 1)
- : this._freeze
- ? new THREE.Vector2(0, 0)
- : this._allowedRotation
+ if (this._force) {
+ return new THREE.Vector2(1, 1)
+ } else if (this._freeze) {
+ return new THREE.Vector2(0, 0)
+ } else {
+ return this._allowedRotation
+ }
}📝 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.
| return this._force | |
| ? new THREE.Vector2(1, 1) | |
| : this._freeze | |
| ? new THREE.Vector2(0, 0) | |
| : this._allowedRotation | |
| get allowedRotation () { | |
| if (this._force) { | |
| return new THREE.Vector2(1, 1) | |
| } else if (this._freeze) { | |
| return new THREE.Vector2(0, 0) | |
| } else { | |
| return this._allowedRotation | |
| } | |
| } |
Summary by CodeRabbit
Release Notes
New Features
Plan2DandPlans2Dclasses for enhanced 2D visualization within the 3D viewer.freezefunctionality to the camera, allowing for temporary halting of movement.Plan2Dobjects.Plan2Dobjects.Improvements
Bug Fixes
These updates enhance user interaction and visualization capabilities in the viewer.