Seems like I misunderstood the representation of org_mitk_timegeometry_timepoints. It needs four values for three time steps (n+1). When reducing the values to three values there's a warning in the terminal:

12.69 core.mod.core.imgIo ERROR: Stored timepoints (2) and size of image time dimension (3) do not match. Switch to ProportionalTimeGeometry fallback

So the actual fix is to change the last value to like 170.

The original timesteps are (ArbitraryTimeGeometry):

min TimeBounds: 
 Step 0: 0 ms
 Step 1: 103 ms
 Step 2: 160 ms

max TimeBounds: 
 Step 0: 103 ms
 Step 1: 160 ms
 Step 2: 160 ms

in another dataset, the navigation spinbox works. Following timesteps:

min TimeBounds: 
        ...
        Step 31: 700 ms

       max TimeBounds: 
       ...
        Step 31: 720 ms

So, I assume that min TimeBounds and max TimeBounds are compared.
Still, the org_mitk_timegeometry_timepoints after converting to nrrd is odd.

I looked around in QmitkImageNavigatorView and QmitkSliderNavigatorWidget, but I didn't stumble over anything suspicious.

One root to this behavior is the fact, that the current time geometries did not support "gaps" between time points and we didn't dare to introduce such a feature without very carefull thinking and evaluation, because it could have a tons of sideeffects and broken dependencies.

This explains e.g.

Still, the org_mitk_timegeometry_timepoints after converting to nrrd is odd.
[...] org_mitk_timegeometry_timepoints:=0 103 160 160

To currently represent a time geometry without gaps and with n time points, we need n+1 values. The first value is the min time bound of the first time point, the next n values are the maximum bounds of all time points.
In Dicom we have a lot of data sets, where a whole volume is encoded to cover just one point in time. So the original information read from DICOM would be [min bound j, max bound j]:
[0,0] [103,103] [160, 160]

Because MITK currently allows no gaps, it is converted in:
[0,103] [103,160] [160, 160]
And underlined values are stored in nrrd to define the geometry.

So basically in the final step we represent whats in the DICOM. That was also the reason why we do not artifically change the last max bound to something like +inf or assime the same duration like the preceeding time step. We did not want to alter the data because that could have implication we can not forsee now.

My bet is that the widget have problems with the fact thet the last time step is infitesimal small. May be just a ">" check and not a ">=" check on the minimal bound of a time step to check if its within the time step.
So basically I think currently we should check that all busiiness code checks time bounds of geometry time steps like a closed open set: [min bound, max bound). Meaning that min bound is within the coverage of a time step and max bound itself is not within the range of the time step

In T24766#131411, @floca wrote:

So basically I think currently we should check that all busiiness code checks time bounds of geometry time steps like a closed open set: [min bound, max bound). Meaning that min bound is within the coverage of a time step and max bound itself is not within the range of the time step

This is at least how it is internally implemented in the ArbitraryTimeGeometry if you request the time step for a certain time point.

Any more ideas on this? I stumbled across this while investigating another DICOM problem.

This does not happen with the new mxn multi widget. Maybe connected to the render windows being synchronized / plane nodes when using the StdMultiWidget?

Edit: The mxn multi widget does not display different time points at all. So changing the time slider / time spin box does not affect the visualization of the data.

I think the problem is here: https://phabricator.mitk.org/source/mitk/browse/master/Modules/Core/src/DataManagement/mitkArbitraryTimeGeometry.cpp$124
The m_MaximumTimePoints for the 3D+t-Heart image is [103, 160, 160]. The last timePoint argument is 160. So in the first iteration if (timePoint < *pos) will be false. Next two iterations it will be false as well since 160 is not less than 160. This will lead to result being 0 at the end of the function.
Normally the m_MaximumTimePoints for another time-image is something like [41, 46, 51] and the timePoint argument will be 46 for the last timestep.
This could be solved by using something like this:

mitk::TimeStepType mitk::ArbitraryTimeGeometry::TimePointToTimeStep(TimePointType timePoint) const
{
  mitk::TimeStepType result = 0;

  if (timePoint >= GetMinimumTimePoint())
  {
    for (auto pos = m_MaximumTimePoints.cbegin(); pos != m_MaximumTimePoints.cend(); ++pos)
    {
      if (timePoint < *pos)
      {
        break;
      }
      result++;
    }
  }

  return result;
}

So we are increase result continuously. However, I'm not sure about any side effects that might come with this (e.g. invalid timePoint, that should resolve to timestep 0 instead of the last timestep).

Using the modified function T26276 could be solved.

@kalali: Sharp eyes and good work. As far as I can see this is a proper solution to fix a state I had not covered in my implementation. The handling of invalid timepoints is still okay. As the documentation only guaranties that negative invalids will be 0. For all others there are no garanties and developers should use GetTimeBounds() to check wether they request legal time points or not.

If you add the fix, please also extend the unit test for ArbitraryTimeGeometry to cover such a scenario as well. (So the new test should faile with the old code, but work with your fix.) Thanks.

Pushed new branch T24766-Fix-last-time-step-access.

@kalali Resolved?

In T24766#131422, @floca wrote:

In T24766#131411, @floca wrote:

So basically I think currently we should check that all busiiness code checks time bounds of geometry time steps like a closed open set: [min bound, max bound). Meaning that min bound is within the coverage of a time step and max bound itself is not within the range of the time step

This is at least how it is internally implemented in the ArbitraryTimeGeometry if you request the time step for a certain time point.

In hindside I would argument differently. It is correct that the implementation of ArbitraryTimeGeometry and PropertionalTimeGeometry implement it that way. But the documentation of TimeGeometry defines it as closed interval and this is also how the spatial bound checks are implemented. It makes sense to keep everything in the same behavior. See also T28262 for that reflection.