Enable custom resolution #148
Conversation
| selectedResolutionX: 0, | ||
| selectedResolutionY: 0, | ||
| selectedResolutionZ: 0, | ||
| voxelSpace: true |
There was a problem hiding this comment.
variable should be more specific. either useVoxelSpace or useVoxelSpaceFlag to indicate it is a boolean.
currently, this variable can be misunderstood as a custom coordinate space
| @@ -296,7 +353,12 @@ export default { | |||
| scaleMax: 10, | |||
| scaleStep: 0.01, | |||
|
|
|||
| testValue: 0 | |||
| testValue: 0, | |||
| selectedResolutionX: 0, | ||
| selectedResolutionY: 0, | ||
| selectedResolutionZ: 0, |
There was a problem hiding this comment.
I think these should be computed properties, rather than data.
They can be computed based on scale and selectedIncomingVolumeResolution{X,Y,Z}
| 'selectedIncomingVolumeResolution', | ||
| 'selectedIncomingVolumeSize' |
There was a problem hiding this comment.
I thought these are already available in selectedIncomingVolume ? I could be wrong
| @@ -328,6 +392,7 @@ export default { | |||
| return this.testScale[0] | |||
| }, | |||
| set: function (value) { | |||
| this.selectedResolutionX = Math.round(+this.testScaleX * this.selectedIncomingVolumeResolutionX) | |||
There was a problem hiding this comment.
ditto comment above (that selectedResolution{X,Y,Z} should be computed property)
if they are computed property, you will not need to manually set them.
| this.selectedResolutionY = Math.round(+this.testScaleY * this.selectedIncomingVolumeResolutionY | ||
|
|
||
| ) |
| @@ -350,6 +418,7 @@ export default { | |||
| return this.testScale[2] | |||
| }, | |||
| set: function (value) { | |||
| this.selectedResolutionZ = Math.round(+this.testScaleZ * this.selectedIncomingVolumeResolutionZ) | |||
| @@ -610,7 +710,43 @@ export default { | |||
| name: `flip on axis ${axis}` | |||
| }) | |||
| this.flipAxis({ axis }) | |||
| }, | |||
| setResolution: function () { | |||
| const isotropic = +this.selectedResolutionX === +this.selectedResolutionY && +this.selectedResolutionY === +this.selectedResolutionZ | |||
There was a problem hiding this comment.
I am somewhat confused.
isotropic is a state that's set by the user, not computed on the fly.
This should be a simple check if (this.isotropic) ? Or am I missing something?
| if (isotropic) { | ||
| this.setIsotropicScale(+this.selectedResolutionX/this.selectedIncomingVolumeResolutionX) | ||
| } else { | ||
| this.isotropic = false |
There was a problem hiding this comment.
ditto above.
this.isotropic should be a state that is exclusively controlled by the user. the application should not be able to influence it.
| @@ -610,7 +710,43 @@ export default { | |||
| name: `flip on axis ${axis}` | |||
| }) | |||
| this.flipAxis({ axis }) | |||
| }, | |||
| setResolution: function () { | |||
There was a problem hiding this comment.
[minor point] it is slightly awkward to use the oninput to trigger state update.
It would probably more declarative to use a watch: { selectedResolutionX }, or indeed, if we are using computed properties, a setter method for the computed property to trigger state update.
semantically, it makes more sense (e.g. anything in the component can trigger a method call. But if the update state is hooked to a watch hook or setter, then only value update triggers the state update)
|
@fsdavid , on further contemplation, since we are strapped for time, we can merge this. (also since we will, one day, swap out with the new version, so the commits likely won't be permanent either.) (Apologies if you have already spent time on this) |
|
Nice, I will merge |
No description provided.