Temporal gui fixes #40787
Temporal gui fixes #40787
Conversation
Because the spinbox had it's number of decimals set to 3 (in ui file) BUT the minimum set (in code)was much smaller, it was possible to set the spinbox to 0.000 (becoming zero) raising strange issues further on. This commit sets both the number of decimals AND the minimum of the spinbox in the code, so it is clear that they should stay 'in sync'
When using timestep < 1s did not work because logic was working for seconds only (adding 0 sec in case of milliseconds). With these changes we add milliseconds to the timeframe, AND we show them in the Temporal Controller ui (only if the timestep-units ARE milliseconds
Because the TimeSlider's last frame is the frame BEGINNING with the last timestamp of the mTemporalExtents, we are actually requesting a frame outside the mTemporalExtents. This commit adds the amount of time to the calculated end so it's size is actually not zero but the exact timestep size.
|
Checking tests |
|
Mmm, I fixed checks, but accidently added a file in which I was trying out something else. Tried to fix that by removing that commit and force pushing, and now... I'm lost :-( |
Regarding the CI, it looks not related to your modifications
You can close and reopen the PR so the CI is triggered again. |
|
@rduivenvoorde first two commits look good, thanks I'm not convinced on 78b374a though -- to me this makes the result misleading, as the animation won't actually respect the user-set maximum time value and will instead show a "bit more" past the end of this value. The consequences of that are hard to determine, but it potentially makes for misleading/inaccurate results, which should be avoided. Will #40371 help with this? |
|
Thanks for checking this! @nyalldawson @Samweli about 78b374a I think this really all depends on the use and use-case. I'm trying to think out the different (data) use cases myself here. And actually come to the conclusion that one of my older pull requests #39568 actually is also not fitting all use cases... |
The old implementation of finding the best fit for the old timeframe was by looping over ALL possible timeframes, raising issues when the user selected milliseconds in a dataset with a rather large Range-extent This new implementation finds a better rough frame to start so the loop should be ended within a short time. The service from #40786 issue is now super quick.
| // earlier we looped from frame 0 till totalFrameCount() here, but this loop grew gigantic if you switched to for example milliseconds | ||
| // that is why now we calculate a rough framestart | ||
| long long roughFrameStart = std::floor( ( frameStart - mTemporalExtents.begin() ).seconds() / mFrameDuration.seconds() ); | ||
| for ( long i = roughFrameStart; i < totalFrameCount(); ++i ) |
There was a problem hiding this comment.
Won't we potentially need to check backwards in some circumstances here? Eg. if the initial guess was too large and the frame estimated actually sits after the desired frameStart, then we'd need to step backwards from the guess until the candidate frame start is <= frameStart...
But otherwise, good approach to fixing this!
There was a problem hiding this comment.
@nyalldawson my gut feeling is that this will never happen, because of the 'floor', AND the fact that you only use the begin of the old frame in combination with a timestep-frame with a length >0
BUT I'm not 100% sure... but in my experiments this all worked.
I can add a comment or todo?
|
This is becoming a mess I think, will close it and create a new PR later with only:
|
These 3 commits fix some smaller glitches in the temporal GUI part of QGIS.
The first one fixes the fact that you can create a step of zero time-units in the gui.
Either by editing the input: type 0 and enter.
Or by using the little down-arrow when Step is 1.000:
Setting a timestep of zero makes no sense (as at that moment you have a TimeFrame of zero, and you cannot step through your data anymore with the slider).
The second one a makes working with MilliSeconds actually work.
In the code the amount of 'seconds' was added to the earlier step datetime. But if you had choosen that the timestep-unit was milliseconds the logic added 'zero' to the step.
Also in the gui you could not see the 'size' of the frame because the start- and end-datetime were formatted WITHOUT milliseconds:
Now the code takes ms into account AND actually shows them (but only IF the step unit is actually ms):
The third one fixes Temporal Controller ends with a Frame with Step size == zero #40777: if you moved the slider to the last timeframe, the size of the timeframe shrunk to a zerosize timeframe:
this was because the logic did not add the 'timestep' to the last datetime of the project extents (because it is indeed 'outside' the project extent....). Now IF we are the last frame we add the actual timestep to it, so it looks better and creates a real filter: