-
Notifications
You must be signed in to change notification settings - Fork 65
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
fix(ruler): avoid error when manipulator returns no worldCoords #474
Conversation
✅ Deploy Preview for volview-dev ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
This looks to be an oversight on how we use worldCoords after the vtk.js@v28 upgrade.
It sounds like all of the widgets need to be checked for their usage of |
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.
Overall LGTM
src/vtk/PaintWidget/behavior.ts
Outdated
|
||
if (!worldCoords) { | ||
console.warn('Event cannot be converted to world coordinates'); |
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.
Redundant console.warn
if (!worldCoords) | ||
console.warn('Event cannot be converted to world coordinates'); |
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.
I think our current manipulators shouldn't trigger this unless there's an error case, so this is a good add. If we start getting them legitimately, then we can remove it.
LGTM! |
Seems that Paint is hurting too...