Drag and drop ability on viewer #9
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ombr
reviewed
Oct 22, 2024
zfir
reviewed
Oct 22, 2024
zfir
reviewed
Oct 29, 2024
There was a problem hiding this comment.
@parveshneedhoo Can we check if we can move the controls to another class
HashirRajah
approved these changes
Nov 4, 2024
| @@ -28,11 +28,11 @@ | |||
| <tbody id="measurements"></tbody> | |||
| </table> | |||
| </div> | |||
| <div id="loading"> | |||
| <div id="loading" style="display: none;"> | |||
| import { Measurements } from "./measurements"; | ||
|
|
||
| export class DragControlHandler { | ||
| private control: DragControls; |
There was a problem hiding this comment.
Suggested change
| private control: DragControls; | |
| private controls: DragControls; |
ombr
approved these changes
Nov 5, 2024
zfir
reviewed
Nov 7, 2024
| new THREE.BufferGeometry().setFromPoints([sphere1.position, sphere2.position]), | ||
| new THREE.LineBasicMaterial({ color }) | ||
| ), | ||
| color, |
There was a problem hiding this comment.
Suggested change
| color, | |
| this.generatedColor, |
Comment on lines
62
to
+63
| window.addEventListener("unload", this.cleanup.bind(this)); | ||
| container.addEventListener("click", this.onMouseClick.bind(this), false); | ||
| container.addEventListener("mousedown", this.onMouseDown.bind(this), false); |
There was a problem hiding this comment.
Not sure if better to use Arrow funcctions here. Can be done in another PR
zfir
reviewed
Nov 11, 2024
There was a problem hiding this comment.
- Should be handle all the events from loader and measurement in Drag Control Handler?
- Loader -> Measurement -> DragControlHandler : I think we handle the classes like and for example DragControlHandler should not have loader as a global variable
I will merge it like this. The comments can be handled in the next PR.
Reviewed with @parveshneedhoo
| renderer: THREE.WebGLRenderer, | ||
| private loader: Loader, | ||
| private onDrag: (object: THREE.Mesh<THREE.SphereGeometry, THREE.MeshBasicMaterial>) => void, | ||
| private onDragEnd: (object: THREE.Mesh<THREE.SphereGeometry, THREE.MeshBasicMaterial>, event: any) => void, |
There was a problem hiding this comment.
Suggested change
| private onDragEnd: (object: THREE.Mesh<THREE.SphereGeometry, THREE.MeshBasicMaterial>, event: any) => void, | |
| private onDragEnd: (object: THREE.Mesh<THREE.SphereGeometry, THREE.MeshBasicMaterial>, event: Event) => void, |
| private loader: Loader, | ||
| private onDrag: (object: THREE.Mesh<THREE.SphereGeometry, THREE.MeshBasicMaterial>) => void, | ||
| private onDragEnd: (object: THREE.Mesh<THREE.SphereGeometry, THREE.MeshBasicMaterial>, event: any) => void, | ||
| measurements: Measurements |
There was a problem hiding this comment.
Suggested change
| measurements: Measurements | |
| measurements: Measurements |
Move above privates
| window.addEventListener('mousedown', this.onMouseDown); | ||
| this.loader.lockRotationAndClick(); | ||
|
|
||
| measurement.lastIntersectedPosition = event.object.position.clone(); |
|
|
||
| private handleDragEnd(event: THREE.Event, measurement: Measurements) { | ||
| const onMouseUp = (mouseEvent: MouseEvent) => { | ||
| measurement.updateMouse(mouseEvent); |
There was a problem hiding this comment.
Suggested change
| measurement.updateMouse(mouseEvent); | |
| measurement.updateMouse(mouseEvent); |
use measurement in measurement
| @@ -54,11 +56,45 @@ export class Loader { | |||
|
|
|||
| container.appendChild(this.renderer.domElement); | |||
|
|
|||
| this.measurement = new Measurements(this.scene); | |||
| this.measurement = new Measurements(this.scene, this.camera, this.renderer, this.group, this); | |||
There was a problem hiding this comment.
Suggested change
| this.measurement = new Measurements(this.scene, this.camera, this.renderer, this.group, this); | |
| this.measurement = new Measurements(this.scene, this.camera, this.renderer, this.group); |
Handle all events in drag-control-handler
| sphere2, | ||
| line: new THREE.Line( | ||
| new THREE.BufferGeometry().setFromPoints([sphere1.position, sphere2.position]), | ||
| new THREE.LineBasicMaterial({ color }) |
There was a problem hiding this comment.
Suggested change
| new THREE.LineBasicMaterial({ color }) | |
| new THREE.LineBasicMaterial({ color: this.generatedColor }) |
Comment on lines
+141
to
+143
| this.spherePairs = []; | ||
| this.currentSpheres = []; | ||
| this.generatedColor = undefined; |
There was a problem hiding this comment.
Duplication should we move that to a new function
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Right now, on the 3D-viewer measurement, we can place two spheres to measure the distance between two points. In this PR, we will add the ability to drag the sphere once it is placed.