BUG: Fix off-by-one bug in end frame for 3D registration #127
Conversation
sbelsk
force-pushed
the
fix-off-by-one-registration-end-frame
branch
from
c04b16a to
ee0b9e9
Compare
| @@ -504,7 +504,7 @@ def registerSequence( | |||
|
|
|||
| try: | |||
| self.isRunning = True | |||
| for idx in range(startFrame, endFrame): | |||
| for idx in range(startFrame, endFrame + 1): | |||
There was a problem hiding this comment.
Considering endFrame is set to the value of CTSelectorNode.GetNumberOfDataNodes(), using range(startFrame, endFrame) seems to be correct.
Indeed, considering the items in a sequence are retrieved starting with 0, calling self.autoscoperLogic.getItemInSequence(ctSequence, idx) with idx set to the number of item would not be correct.
There was a problem hiding this comment.
Also extending the list nodeList while it is being iterated over does not seem to be correct.
This pattern seems to be used in a few location
There was a problem hiding this comment.
while it is being iterated over does not seem to be correct.
To follow-up offline discussion with @sbelsk, since this code was known to work, this will likely be reviewed/addressed in follow-up pull requests.
There was a problem hiding this comment.
Considering
endFrameis set to the value ofCTSelectorNode.GetNumberOfDataNodes(), usingrange(startFrame, endFrame)seems to be correct.Indeed, considering the items in a sequence are retrieved starting with 0, calling
self.autoscoperLogic.getItemInSequence(ctSequence, idx)with idx set to the number of item would not be correct.
Makes sense, thanks for pointing out the inconsistency. The UI frame selector slider was set to go from minimum=0 to up to maximum=CTSelectorNode.GetNumberOfDataNodes() which would add one more frame than there really is.
I pushed a fix to make sure the maximum value the user is able to select is actually the last frame in the sequence. I believe all other places in the code use CTSelectorNode.GetNumberOfDataNodes() consistently such that the first frame is treated as index 0 and the last is treated as GetNumberOfDataNodes()-1. (E.g., error checks in TreeNode.py of the form if idx >= self.transformSequence.GetNumberOfDataNodes(): and the loop that goes for idx in range(self.transformSequence.GetNumberOfDataNodes()):).
|
good find @sbelsk |
sbelsk
force-pushed
the
fix-off-by-one-registration-end-frame
branch
from
ee0b9e9 to
e6005b5
Compare
The UI slider in the 3DHierarchicalRegistration module for the range of frames to be registered implies that the selected interval is inclusive of the end frame, which is evident also in the number of frames in the initialized transform sequence. However, the registration used to be run on all frames except the last one, so the last frame in the resulting transform sequence was always left as the initialized identity. This commit fixes this off-by-one bug in the frames being registered.
jcfr
force-pushed
the
fix-off-by-one-registration-end-frame
branch
from
e6005b5 to
9dbe212
Compare
The UI slider in the 3DHierarchicalRegistration module for the range of frames to be registered implies that the selected interval is inclusive of the end frame, which is evident also in the number of frames in the initialized transform sequence. However, the registration used to be run on all frames except the last one, so the last frame in the resulting transform sequence was always left as the initialized identity.
This commit fixes this off-by-one bug in the frames being registered.