Open laurennlam opened 3 years ago
Maybe it comes from scale3 mixin, since rectangle widget paints correctly and has a corner mixin where ellipse handle doesn't and has a scale3 mixin. Both those mixins are used to paint slices in the paintWidget example
Do you see the paint cursor when over the I/J view? If not, then it sounds like the plane manipulator's normal is still pointing in the K direction.
Does the paint show up in the K view as a stroke, or a single painted dot/circle? If it's a stroke, then the paint coordinates are not being correctly transformed into image space.
Those are my initial thoughts, but the issue could be elsewhere.
Thanks @floryst for your ideas. For tracking issues, here is what I indentified as issues in the Paint Widget example:
@floryst @agirault @laurennlam who should update the plane manipulator ?
As a more general question, how the current viewed slice (and orientation) should be communicated to the widget ?
Hey all. Just letting you know that I've seen the issue but haven't looked at those for a while, so I'll take some time to look at the examples, the last PR that touched those, and your PR, and I'll report back later.
I think the application should update the plane manipulator. The reason is to provide flexibility for how an application wants to paint. If the widget's behavior.js controlled the plane manipulator, then we would have to pick a fixed behavior for the manipulator. In the painting case, the only reasonable fixed behavior is to align the plane normal with the camera direction. By letting the application change the paint manipulator, we can support cases such as painting on a plane that isn't parallel with the view plane (even if such a case is contrived and not very useful).
I've also been running up against those same questions. Here are a few thoughts:
widgetProvider.js
manages widgets and views, and invokes lifecycle events on the widgets. The current slice values are stored in an application store, and changes there update the manipulator for slice-aware widgets.Hey all,
The example seems quite broken (sometimes stuck in windowing instead of annotating??), but I manage to place the shape widgets, they seem to be properly facing I, J or K slices: https://kitware.github.io/vtk-js/examples/ShapeWidget.html
Can you share a video or a minimal example of the specific issue you're encountering? And if I'm out of the loop and Forrest's answer is going towards the direction you need, you can ignore my comments.
Hi, Can you check the Paint widget instead ? On my side, this is the one which fails. https://kitware.github.io/vtk-js/examples/PaintWidget.html I just tried again:
Thanks @laurennlam really helpful
switching axis and trying to paint circleWidget/ellipsewidget doesn't work
I see that, though the actual shape widget orientation seems to work: the green shape is always facing the camera, whether I, J or K). However:
switching several times from I to J to K to I to J,... We notice that the image is rotating (bad viewUp ?)
I saw that too, yeah there's clearly a wrong camera set up in those two examples. I'm not sure if that's related to the issue, but might be worth fixing that slice rendering first to make sure what we test is valid.
It seems that sometimes the green circle is correctly displayed on the view (even when you change slice) but when you release the mouse, nothing happens. To reproduce:
Yep, I see the same thing, I think we're on the same page @laurennlam. From my understanding, the "nothing happens" you're mentioning is when we go from Shape Widget (nicely oriented, though not also properly placed) to Paint Widget, so I'd investigate here. I haven't used the Paint Widget much so maybe @floryst can confirm if my guess is reasonable.
Edit: I initially wrote "Pain" instead of "Paint". Seems fitting 🙃
My bad @agirault , bad reading of what you wrote. Indeed, we're on the same page :)
Regarding, https://github.com/Kitware/vtk-js/issues/1718#issuecomment-773486950 I first agreed with you @floryst 🆗 :
the application should be the one setting the current "slice" (origin+orientation) onto the widget plane manipulator.
Because a widget may be rendered in multiple views (3D, axial, coronal, sagittal, oblique...) at the same time, we can't store the current slice of all the views into the widget state
to be used by the behavior
functions.
However, even if the application sets the widget manipulator, the manipulator will get reset next time a handle is activated 😞 .
Moreover, @agirault has a good point with https://github.com/Kitware/vtk-js/pull/1717#discussion_r571109291 . We should shoot for stand-alone widgets that are easy to use.
🔥 I think what we are missing is to have more context on the view where the widget is being rendered. 🔥
We first encountered that limitation ⛔ with the reslice cursor (see attached email discussion) which led us to add 3 new types of ViewType. It made the job, but we were not very satisfied 😥 with this solution.
I think we should be good if the widget can access some properties of the view it is currently being rendered: e.g. name, renderer, camera, and if it is a "slice" then it's origin and orientation.
What do y'all think 🤔 ? @floryst @agirault @jourdain @laurennlam @ocrossi
If so, what would be the best way to do it ⚙️ ?
I'm slightly lagging behind the issue discussed here. But I'm definitely open to have a meeting and work out the actual issue(s).
Normally the ViewType were meant to map a representation for a given view, not to change behavior or state of a given widget. A widget representation should be able to have access to its view. The split between representation, state, behavior and manipulators was really meant to properly split responsibility and enable code reusability across widgets.
I'm open to do a meeting in french first and then between @agirault and I we can loop in @floryst. But ideally I would prefer if @floryst could attend as he has the most "fresh" experience with widgets.
I think we should be good if the widget can access some properties of the view it is currently being rendered: e.g. name, renderer, camera, and if it is a "slice" then it's origin and orientation.
I think this is a good idea. I do agree that having stand-alone widgets is good, but I also was thinking about how much flexibility should we allow for versus having widgets that work in a restricted manner.
I'm open to do a meeting in french first and then between @agirault and I we can loop in @floryst. But ideally I would prefer if @floryst could attend as he has the most "fresh" experience with widgets.
+1 to meeting over this.
After thinking more about stand-alone widgets vs application-level logic, I realized that we need more infrastructure surrounding widgets. Here's some more thoughts on what I would like to see out of the widgets, or what might make sense for future changes. Just some ideas that may or may not be useful, but might help give other people ideas.
manipulators: the idea to split out how mouse events are handled was a great idea. Widgets can specify a default manipulator (e.g. plane manip for line widget), but the user can change if if they want some richer manipulator functionality. For instance, a user might change the DistanceWidget's default plane manipulator to some manipulator that picks the surface of some geometry.
We are now running up against the problem of how to update the manipulators. For instance, we have widget behavior that assumes the widget always has a plane manipulator that is updated from the camera direction. What if we wanted to instead update the plane manipulator from some other source, like an image mapper?
For a more concrete example, let's consider a ruler widget. By default, it use a PlaneManipulator (params: normal, origin). Now let's add another default: the PlaneManipulatorImageMapperUpdater. As input, it requires an ImageMapper, and it will update the PlaneManipulator whenever the ImageMapper updates. But what if there is no image and the user wants to instead use the plane constructed from the camera focal point and the view direction? Then they should be able to update the widget to use, say, a PlaneManipulatorCameraUpdater that will update the plane manipulator from the camera.
/end of comment that is longer than I intended it to be.
Hi,
I'm working on the PaintWidget with the circle/ellipse widget. We found an issue when we move the slice index which is fixed by this MR. But, even with this fix, when I try to paint on I or J view, it fails and still draw into K plane. I guess that it comes from the commits which replaced direction by orientation but I'm not sure. Does anyone have any idea ? @agirault @floryst