-
Notifications
You must be signed in to change notification settings - Fork 24
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Isosurface picking #3858
Isosurface picking #3858
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very impressive! The hover effect is awesome and makes this feature look and feel very futuristic, great job 👍
Out of interest, how did you solve the problem you had during development, where the hit test did not work all the time? With the clippingOffsetFactor
?
@@ -183,6 +183,7 @@ export function rotate3DViewTo(id: OrthoView, animate: boolean = true): void { | |||
const pos = voxelToNm(dataset.dataSource.scale, getPosition(state.flycam)); | |||
|
|||
const aspectRatio = getInputCatcherAspectRatio(state, OrthoViews.TDView); | |||
const clippingOffsetFactor = 100000; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add a comment explaining why this is needed? :)
); | ||
|
||
raycaster.setFromCamera(mouse, this.cameras[OrthoViews.TDView]); | ||
const intersections = raycaster.intersectObjects(isosurfacesRootGroup.children, true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need to intersect recursively? Is this because the segments for one id are in a group or is there another reason?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it's because the segments are in a group and the group is only traversed if recursive
is true. I'll add a comment!
@@ -137,6 +143,69 @@ class PlaneView { | |||
} | |||
} | |||
|
|||
performIsosurfaceHitTest(): ?THREE.Vector3 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you experiment with throttling this function? To me, during testing, the TD view felt a little sluggish and I was wondering whether this could help to make it feel more performant :) I do understand that the "instantness" of the hover effect contributes to the "coolness" of the future and I do want to keep that, so maybe just throttling it a little is enough.
It's not a blocker though as the performance hit is only noticeable when isosurfaces are loaded.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are absolutely right. I definitely needed to add throttling, as it doesn't make sense to compute this on every render. I opted for 150 ms as a throttling delay, which felt like a good trade-off between performance and delay to me.
URL of deployed dev instance (used for testing):
Steps to test:
Issues: