-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[FEATURE][MESH] plug mesh layer to QGIS temporal framework #35466
Conversation
if ( datasetTimes.isEmpty() ) | ||
return QgsMeshDatasetIndex(); | ||
|
||
// If requested time is before the timestamp of the first dataset, return the first dataset |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure about this one -- shouldn't we return an "invalid" dataset instead so that nothing gets rendered?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With mesh layer, some datasets (for example, terrain elevation) are not really temporal and there is no reason to not display them for particular time step. But, MDAL doesn't provide way to distinguish "static" datasets and "dynamic" datasets. I used this workaround to always display such datasets, but, sure, it is not very clean... We can add the condition that the dataset group has only one dataset but it is not completely satisfying...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would argue that this situation should be handled by users disabling the temporal option for those layers - that has the effect that the layer is always shown.
One option that is missing here is an equivalent of the raster temporal properties "fixed time range" mode, where a user can manually set a particular time range for that layer to shown within. It's in place to allow the animation framework to work with non-temporal-aware data sources (e.g. geotiff) where a user may have a set of multiple layers representing different time periods, and wants the animation to automatically switch them on or off if they fall within this fixed time range.
I think the mesh layers could also benefit from this option -- it's a "last resort" fallback for users allowing maximum flexibility for animations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Set the whole layer static could be a solution for the user, but for mesh layer, there is not only one dataset group, we can have a terrain elevation dataset and many other dynamic dataset groups. I think also about the min/max dataset group...
More over, the problem is the same when the request time is greater than the time extent of the datasets. That is like this because the request of dataset is done with only one relative time value.
I wonder if it will not be better to request dataset with interval of relative time, and add an option to allow the display with the extreme dataset when the request time interval is before or after the time extent of the datasets?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
discussed with Vincent:
- implement
MDAL_EXPORT bool MDAL_G_isTemporal( DatasetGroupH group );
- add
isTemporal()
to QgsMeshDatasetGroup class - check the code that if group is not temporal, display always regardless of the time settings
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as for "fixed time range" mode, I would propose to have it available (optionally) only for static mesh layers. for dynamic I do not see much sense
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I'll defer to your superior knowledge of mesh internals here! 👍
Looking great! It's nice to see the temporal capabilities being brought into the 3d world too!
I think for consistency with raster layers this should be done |
nice work! Already did some pre-review in the PR in Vincent's fork before official PR |
|
||
void clear(); | ||
%Docstring | ||
Clears alls stored reference times and dataset times |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clears alls stored reference times and dataset times | |
Clears all stored reference times and dataset times |
.. versionadded:: 3.14 | ||
%End | ||
|
||
virtual const QgsDataProviderTemporalCapabilities *temporalCapabilities() const; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't forget to update the existing raster and vector data providers to implement the const version too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and SIP_SKIP, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, correct.
Thinking about this - it might be nicer to make the const version non virtual:
const QgsDataProviderTemporalCapabilities* temporalCapabilities() const { return const_cast< QgsDataProvider* >( this )->temporalCapabilities(); }
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, very nice and smart!
But, a little cons, there will be no visibility of this const method in the derived class... easier to implement but maybe future issue ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I'm not sure. Something feels "off" about the double virtual method though...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Finally, as the compiler looks to no like very much this elegant way, I kept the first choice.
@@ -5470,6 +5471,15 @@ QgsMeshLayer *QgisApp::addMeshLayerPrivate( const QString &url, const QString &b | |||
return nullptr; | |||
} | |||
|
|||
// Manage default reference time, if not reference time is present by default in the layer -> assign one | |||
if ( ! layer->temporalProperties()->referenceTime().isValid() ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good for now! If there's more need for this in future we could consider adding a method to the layer temporal properties, something like "setDefaultsFromProject( QgsProject* project )" which we call here instead, but for now this is fine.
if ( datasetTimes.isEmpty() ) | ||
return QgsMeshDatasetIndex(); | ||
|
||
// If requested time is before the timestamp of the first dataset, return the first dataset |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would argue that this situation should be handled by users disabling the temporal option for those layers - that has the effect that the layer is always shown.
One option that is missing here is an equivalent of the raster temporal properties "fixed time range" mode, where a user can manually set a particular time range for that layer to shown within. It's in place to allow the animation framework to work with non-temporal-aware data sources (e.g. geotiff) where a user may have a set of multiple layers representing different time periods, and wants the animation to automatically switch them on or off if they fall within this fixed time range.
I think the mesh layers could also benefit from this option -- it's a "last resort" fallback for users allowing maximum flexibility for animations.
Looks good to me -- apart from the typo and const note above I'm +1 to merge |
Now, dataset group can be temporal or not. For now, non temporal are defined in the data provider when there is only one dataset because MDAL doesn't support this feature. Next version of MDAL will support it and the provider will be modified to take account. |
Looks good to me, but I can't comment on the mesh specifics. I'm +1 to merge, but let's give @PeterPetrik a final chance to look over... |
@vcloarec there is failing test for mesh3d
|
@PeterPetrik time to merge? |
Nice to see the disjointed temporal stuff starting to come together :) |
@vcloarec |
Time handling in mesh layer
The time in a mesh layer is defined by :
The class
QgsMeshDataprovidertemporalCapabilities
stores the reference time provided by the data and all the relative times of the dataset. This class has the ability to return dataset index from a dataset group index and a relative time since the reference time.The reference time (can be different than the provider reference time) and the absolute time extent are stored in the class
QgsMeshTemporalProperties
The temporal settings in the properties widget are only the reference time and the provider time unit:
The default reference time of the layer is set by (sorted by priority):
The user can change it if he wants.
Rendering principle
The 2D and 3D renderers access to the active dataset index from the layer with the time range stored in the
QgsContextRenderer
and in theQgs3DMapSettings
:Relative time is calculated with difference between the time range of
QgsContextRenderer
and the reference time stored byQgsMeshTemporalProperties
. The dataset index come from theQgsMeshDataproviderTemporalCapabilities
that maps the relative time with dataset index.Then data are brought from the provider with the dataset index.
Static dataset
There is also the possibility to set static dataset, that is to choose the dataset that will be rendered independently of the QGIS time controller. To do that the user can check the check box that is is at the bottom of the temporal page of the properties widget:
For now, as that was simpler to implement and allows to choose directly wanted dataset, the used can choose independently scalar dataset and vector dataset from combo boxes with the time associated to each dataset (relative time for data without reference time). If wanted, a unique time could be implemented, but that's could lead to inconsistently results with non synchronous dataset.
The static dataset settings are in the temporal page but can be easily put in the source page if wanted.