search

neuroanatomy / thresholdmann

A tool to apply a smoothly varying threshold to a nifti image
https://neuroanatomy.github.io/thresholdmann/index.html
ISC License
3 stars 2 forks source link

[joss][ui] Restrict control points to be within the borders of the image #26

Closed sneakers-the-rat closed 5 months ago

sneakers-the-rat commented 6 months ago

Part of: https://github.com/openjournals/joss-reviews/issues/6336

Ok this one i find extremely charming.

You can drag the points outside the image, onto the menus, and it... works.

https://github.com/neuroanatomy/thresholdmann/assets/12961499/3d89eb7a-491b-4f06-b6d2-488568827519

So as much as i literally love that (bravo on making the code robust enough to just casually handle that, the points don't even disappear behind the menus), it would probably not be great to be able to do that since it would create some undefined behavior in downstream uses of the control points (what does it mean for the point to be in negative voxel space, or in a voxel larger than the size of the dimension?)

katjaq commented 5 months ago

:D Thanks for spotting this! And for your video for illustration (and eternity :D) May we call it a feature ? 🙃 It is like any control point – just "far away by a certain amount" and hence the values will be handled accordingly. :) Voxels outside the brain, just negative coordinate, don't mean anything. :D The points don't disappear behind the menu and you can even catch the point once it is outside, you can bring it back in ... ... ya, you are right, we should probably restrict the space. 🙈

sneakers-the-rat commented 5 months ago

I'll leave this one up to you - it is one of those things that's like... probably fine? and you're right, it is technically valid as far as setting a parametric threshold map goes, since the 0 point is arbitrary anyway? and it doesn't break the interface, and the tool handles it just fine, so i think the only concern is downstream usage? so maybe a tickbox to "constrain points within volume" or something for if that becomes a problem for ppl lol

katjaq commented 5 months ago

Thanks @sneakers-the-rat . <3 We have now restricted control points to remain within the bounding box in PR #32 – they will stay inside, closest to the cursor, in case it leaves the data.

sneakers-the-rat commented 5 months ago

hahaha ok great, probably for the best, but save this to some "lovable bugs" folder