-
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
Add support for transformations with thin plate splines #7131
Conversation
@fm3 As discussed, I imagine the following format for the new tps transform type: {
type: "thin_plate_spline";
correspondences: { source: Array<[number, number, number]>; target: Array<[number, number, number]> };
} Could you adapt the back-end accordingly? 🙂 |
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.
Looks very good! I'm impressed by how relatively few changes were needed to support this 👍
It also worked well with the dataset you supplied. For further testing, it would be great to get a dataset with a scale other than [1, 1, 1], preferably anisotropic.
@@ -15,6 +15,7 @@ For upgrade instructions, please check the [migration guide](MIGRATIONS.released | |||
- Added ability to view [zarr v3](https://zarr-specs.readthedocs.io/en/latest/v3/core/v3.0.html) datasets. [#7079](https://github.com/scalableminds/webknossos/pull/7079) | |||
- Added an index structure for volume annotation segments, in preparation for per-segment statistics. [#7063](https://github.com/scalableminds/webknossos/pull/7063) | |||
- Instead of showing all possible action items next to each other, there is now an overflow menu for layer actions. [#7123](https://github.com/scalableminds/webknossos/pull/7123) | |||
- Added support for transformations with thin plate splines. [#7131](https://github.com/scalableminds/webknossos/pull/7131) |
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.
Should we add a small section to the docs on how to use those and link it here?
@@ -399,6 +402,12 @@ class PlaneMaterialFactory { | |||
}; | |||
|
|||
this.material.side = THREE.DoubleSide; | |||
this.startListening(); | |||
|
|||
this.recomputeShaders(); |
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.
Does this mean that if TPS are used, the first shader compilations are in vain and the actually useful compilation is triggered, here once the TPS are initialized? 🤔
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 right, this is not ideal. I pushed a commit which works around this. Still not 100% satisfied with the solution, but I don't have a better idea for now..
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.
Sorry, but I'm having trouble following the workaround, maybe because I didn't fully grasp the original issue. Could you add some context on what the issue is and how your workaround solved it?
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.
Sure, I see that the commit doesn't make this really clear and also was unnecessarily complex. I changed the code again so that basically the only remaining change is that the uniform listeners are set up before the initial shader code is generated. The reason for this is that one of the uniform listeners also mutates this.scaledTpsInvPerLayer
and that property is necessary to compute the shader code. It's not super elegant, as I said before, but it does the trick 🙈
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.
Thanks, that makes sense 👍
frontend/javascripts/oxalis/model/accessors/dataset_accessor.ts
Outdated
Show resolved
Hide resolved
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.
LGTM 👍
Since this is a niche feature, it would also be alright with me to test the anisotropic case in production - your call.
Adds support for a thin-plate-spline transform which is defined as a list of correspondences (source and target points). An (inverse) TPS is found which fits these correspondences. The resulting weights are passed to the shader which then uses these to transform the coordinates ad-hoc.
The rest of the code (mostly the bucket pickers) don't use the TPS, but instead use an affine approximation which is also derived from the correspondences. The downside of this approach is that the bucket picking can get inaccurate if the affine approximation deviates from the actual TPS transformation. However, in the one known demo case, this is not an issue, which is why I'd keep it as is for now.
Steps to test:
Issues:
(Please delete unneeded items, merge only when none are left open)