It's mostly a paranoid check, since it's used to interpolate the velocity in the loop below.

Other times would work correctly, though memory usage wasn't what I have in mind here. It's more related to the TODO noted in blender_mesh.cpp, about only assuming that two times are supported. To get motion blur information Cycles changes frame and loads the object which will not work for fluid meshes, so we have to interpolate the data in the same frame. In that respect this check is a bit bogus since all the work will be done at time -1.0, there's no need to check for time 1.0.

I'll merge the two functions together and we'll go from there.

Yes, it doesn't have any purpose. It's from some failed test I made, it should have been removed.

This is wrong, motion_time will always be -1.0f, to make it work properly, and to be future proof, I think we could use the motion_times set, then we will not have to worry about the code only assuming 2 motion times. What do you think here?

To get the result in the first attached image, I manually set motion_time to 1.0f at the end of the loop, which isn't nice.

I don't understand what you propose to change, perhaps the code for the change would help me understand it.

Thinking about this more it's not clear to me how -1.0f or 1.0f are working at all, I think in those cases we are evaluating different frames and the mesh topology can be totally different. Should we be exporting the vertex velocities immediately along with the vertex positions, instead of doing this as part of regular motion blur export?

I'm usually bad at explaining, so bear with me :)

Yes, we should be exporting the velocities along with the vertex positions. And that's what is happening here. The interpolation for all motion times is done in one go (not by changing frame and using delta positions). We do that on the first time/frame, so there is no risk to interpolate with a different mesh, and the velocities coincide with the vertices. Thus the check above (line 962) for the time_index (if time_index == 0, then it's the first frame, otherwise return from function since we already have the data).

In non fluid cases, data is copied as required on frame change (because the mesh topology is the same).

Currently the code does it somewhat properly: we do interpolate for all motion times, but since we do it on the first frame, motion_time is set to -1.0f and is never changed. The motion_time parameter is passed all the way up from sync_motion() (which loops over the all motion times and changes scene frame each iteration), so I would like to do something similar here:

foreach(float relative_time : motion_times) {
    float3 *mP = attr_mP->data_float3() + step*numverts;
    float3 *mN = (attr_mN) ? attr_mN->data_float3() + step*numverts : NULL;

    for (b_fluid_domain.fluid_mesh_vertices.begin(fvi), i = 0; fvi != b_fluid_domain.fluid_mesh_vertices.end(); ++fvi, ++i) {
        float3 nPos = P[i] + get_float3(fvi->velocity())*relative_time*shuttertime*0.5f;
        mP[i] = nPos;

        if(mN) {
             mN[i] = N[i];
        }
    }
}

This will ensure that we do the interpolation for the right number of motion_times with the right values, and all in one go (without frame change).

Note that motion_time is not used in the non-fluid case, it's used here to change the direction of velocity (-1.0 will go backward, 1.0 forward), so we have a somewhat nice interpolation in time/space.