Fix #48942 add 1 timestep to AnimationRange

[rduivenvoorde]

Description

This small change will Fix #48942

The TemporalController does not take the last step into account, when loading Mesh layers.

This zip:

123steps.zip

contains 3 netcdf's (ugrid) with 1, 2 and 3 (day) timesteps to test.

WIthout this commit, you will not be able to see the last timestep, without changing the Animation range.

Note that we do the same (add one timestep to the range) for Vector temporal properties:
https://github.com/qgis/QGIS/blob/master/src/core/vector/qgsvectorlayertemporalproperties.cpp#L74

[rduivenvoorde]

Note that having data with just 1 timestep, the 'times' here will be empty, so you will only enter this if when length is at least 2 so times[times.length() - 2] is safe.

[rduivenvoorde]

@PeterPetrik @vcloarec can you maybe guide me a little to fix the test, if you have time? Obviously after this commit, the resulting temporal extents are changed (as I add one timestep), but I'm unclear which parts of the test test_temporal() I can change (I'm not so familiar with this "datasetIndexClosestFromRelativeTime etc" kind of options:
https://github.com/qgis/QGIS/blob/master/tests/src/core/testqgsmeshlayer.cpp#L1950

The crux of this commit: because the temporal controller is working with 'range'-filter, with an INcluding start and an EXcluding end (of the filter range), the last timestep in a Mesh is never shown in the Temporal controller, because the extent calculated earlier was UNTILL the last timestep (after the reference time).
(easily testable with included mini meshes, but also with the meshes from the mdal test data, see #48942)

Sorry if I'm not clear enough...

[vcloarec]

Hi Richard,
Just a quick comment before I look deeper, but not sure it the good place to change the time extent. If you change here, you alter the real time extent of the mesh layer.
Maybe it is better to change the extent where where this time extent is required to be changed, that is at the temporal controller widget level.

[rduivenvoorde]

@vcloarec yes, maybe you are right.

Another (in this case working option) is to add +1 in the TemporalNavigationObject in the 'totalFrameCount()' member:
https://github.com/qgis/QGIS/blob/master/src/core/qgstemporalnavigationobject.cpp#L322
so make it ( +1 added)

return static_cast< long long >( std::ceil( totalAnimationLength.seconds() / mFrameDuration.seconds() ) + 1);

That will always add an 'extra' timestep (in case of other temporal datasets also I think)

@nyalldawson do you have an opinion in this? Both options 'work'.

Crux here: in a netcdf (mesh in general?) you define a 'period since' timestep, so say:

• you define as 'time array: 'years since 1-1-2000'
• then the first data array in it is for say: 1-1-2000 (time = 0)
• the second for 1-1-2001 (time value = 1)
• etc
  Current QGIS will create a controller, starting on 1-1-2000 and ending on 1-1-2002
  But because the end is NOT inclusive, the last timestep isn't shown/visible.

@vcloarec though there is an argument that the stepcount in the meshprovider is off by one...
For example, have a look at one of the dataset:
https://github.com/lutraconsulting/MDAL/blob/master/tests/data/netcdf/indonesia_nc4.nc
using ncdump you see:

...
time:units = "hours since 1900-01-01 00:00:0.0" ;
...
 time = 1008072, 1008096, 1008120, 1008144, 1008168, 1008192, 1008216, 
    1008240, 1008264, 1008288, 1008312, 1008336, 1008360, 1008384, 1008408, 
    1008432, 1008456, 1008480, 1008504, 1008528, 1008552, 1008576, 1008600, 
    1008624, 1008648, 1008672, 1008696, 1008720, 1008744, 1008768, 1008792 ;

There I count 31(!) datasets/timesteps, while if you load this in QGIS, the controller just shows 30(!).

[github-actions]

The QGIS project highly values your contribution and would love to see this work merged! Unfortunately this PR has not had any activity in the last 14 days and is being automatically marked as "stale". If you think this pull request should be merged, please check

• that all unit tests are passing

• that all comments by reviewers have been addressed

• that there is enough information for reviewers, in particular

  • link to any issues which this pull request fixes

  • add a description of workflows which this pull request fixes

  • add screenshots if applicable

• that you have written unit tests where possible
  In case you should have any uncertainty, please leave a comment and we will be happy to help you proceed with this pull request.
  If there is no further activity on this pull request, it will be closed in a week.

[PeterPetrik]

Hmm, this is strange, I would assume it would pick the last step. If you open a similar example in the vector layer, does it show the last step?

[rduivenvoorde]

@PeterPetrik there has been a time (pre 2021) when the temporal filter took both limits into account (INcluding both start en end limit). At that time the mdal filter worked correctly.

Working with measurements with time ranges, I've put a lot of effort to change this, so (the default option) is to include the start limit (of the filter range) and to exclude the end limit (for more discussion see the lengthy conversation of this issue: #38468 and PR #40989 (and the followups of that one).

Argh!! Vector is behaving the same (see the project + data below):

time.zip

It has all to do with the fact that data with just one timestamp column (instantaneous data), in which the event has zero-length (in time), the last step is in that case never shown...
See the tests and comments here for all possible ways:
https://github.com/qgis/QGIS/blob/master/tests/src/python/test_qgsvectorlayertemporalproperties.py#L181
Note that the Temporal controller filter is behaving as it should.
It is just that the Temporal Controller Widget does not give enough frames for the data...

Which would be an argument to add always one extra timestep in totalFrameCount:
https://github.com/qgis/QGIS/blob/master/src/core/qgstemporalnavigationobject.cpp#L313
@nyalldawson what you think?

[PeterPetrik]

Fixing the widget is what @vcloarec suggested too, it should behave the same for vector & mesh, so in case there are more frames we could take the last one too.... Could you make such PR to discuss that implementation instead of this one?

[rduivenvoorde]

@PeterPetrik yes, will do, closing this one. Sorry for the fuzz...