[QGIS-Developer] Temporal controller issues

[Nyall Dawson]

I'm going to respond to your project-level criticism first, and then
following that I'll give a technical response:


On Thu, 7 Jan 2021 at 12:33, Cory Albrecht <maps at hanfastolfe.com> wrote:
>
> >  and that using it for data analysis has been a secondary goal
>
>
> I'm not doing any data analysis. In fact, I doubt that I use even 10% of QGIS's capability.

By analysis I mean: "anything that's not just viewing data".

> > 1. The temporal framework was in discussion and consultation phases
>
> for years prior to implementation.
>
>
> Then how did something as simple as the selection tool selecting stuff that is not there get past this? Did nobody say "Hey, we need to make sure that when the NTC is turned on that other parts of the application work as expected"?

Bingo. You're the first to bring up the issue with the selection and
map tip tools. Guess that shows that we need more people like you
involved with the discussion, testing, and review process.

>
> > 2. QGIS already has some of the most stringent and rigorous code review processes around, and the pull requests implementing the temporal framework were subject to all these processes and peer review.
>
>
> Then how did this problem get past that review? Did nobody take the feature branch, compile it, then run QGIS and do some common, simple tasks regarding data entry with the NTC turned on to see that those tools are working on the “root” layer data and not the filtered data one sees on the map canvas? (Or an automated GUI test harness that does the same thing?) Or was it only tested by loading a project with temporal layers and changing the viewing time frame and seeing that only matching features were drawn on the map canvas?

Of course they did. But no-one anticipated your use case and tested that.

(Hint: YOU could have been this person. There's nothing special about
anyone that qualifies them to be part of a mystical "qgis beta
testing" group. No-one is paid to do that. No-one is elected to do it.
All it takes is downloading the nightlies prior to release and running
them through YOUR workflows. So I reflect this criticism right back on
you -- why didn't YOU beta test QGIS and ensure that it worked for
your workflow? Are YOU going to get involved and ensure this doesn't
happen again? If your answer is "no" then I suggest you take some time
away from this community and come back when you've had a chance to
meditate and reflect on how open-source works.)

>
>
> > 3. If you're asking for the pace of feature development to be slowed
>
> then that problem was addressed years ago when the LTR releases were
>
> introduced.
>
>
> No, not at all. That was purely a semantics comment about not using standard terminology, which makes things confusing.
>
>
> > The QGIS website is quite clear in advertising the LTR as the officially recommended version for stability and for mission critical work, while the non-LTR releases are clearly branded as "bleeding edge".
>
>
> I would disagree about that clarity. When you go to the main page, for example, there's just a download button with text underneath saying "Version 3.16.2 Version 3.10.13 LTR". On the download page it just says "riches on features" and "most stable". And the user manual for 3.16 itself says "We recommend that you use this version over previous releases."
>
>
> I couldn't find anything with the qgis.org search feature that says what you say here.
>
>
> > I'd make the case that in the situation you've described you should never have been using the non-LTR release
>
>
> Unfortunately I've been somewhat forced into using master because other bugs I have reported about how QGIS handles array fields in PostGIS layers. Without those fixes I sometimes couldn't even work on my maps because I couldn't save features without data corruption, or sometimes not even save at all. Those fixes, when they finally got done (and some still aren't), took a while to end up in the LTR.
>
>
> > So I ask again, in the full spirit of reconciliation and moving forward: what concrete, practical changes do YOU think the project should make?
>
>
> Testing, like I mentioned above. Better testing.

Great, thanks for volunteering to do this! Like I said above, we need
more beta testers, and I'm looking forward to your testing and
feedback. (yes, that was semi-sarcastic. But this whole thing only
works when everyone gets involved).


Now the technical part:

> For example, one can add all sorts of SQL-like filters to a layer datasource (which was how the TimeManager plugin worked, if I understand correctly) and the selection tool, vertex tool, and so on are not broken by that. When somebody turns on the NTC, it is reasonable for them to expect that the filtering it does would act in a similar manner to those SQL-like filters added manually to a layer, yes?

Time manager indeed piggy backed onto the layer sql filter
functionality to implement time filtering. This had some advantages in
that the temporal filtering became an innate property of the layer,
but it was ultimately a dead end. There were issues with using the
layer filter which couldn't be resolved, including:
- poor UX -- users could not edit the layer filter while time manager
was active, as the changes would either break time manager's temporal
filter or would get immediately discarded as soon as time manager
changed the temporal part of the filter.
- constant fighting with time formats, and provider-specific support:
because layer sql filters are handed directly off to the underlying
backend, their syntax varies provider by provider. This means that the
time manager approach becomes a tricky/fragile balancing act of
provider-specific filter clauses. This also led to the requirement of
strict time format constraints, which again varied provider by
provider (and sometimes even locale by locale). While that's fine for
a plugin, we wanted to ensure that time handling in qgis core was as
seamless and pain-free as possible, and would work transparently
across all data sources. Thus a different approach was required.

> This is why in my initial email I asked about the pipeline from retrieval of features in the layer's data provider to drawing them on the canvas. Prior to the NTC, did tools like selection, vertex, identify, and so on work off of the same "root" list of features that the map canvas drawer worked from?

No, everything in qgis works on a request-by-request, iterator based
approach. The map rendering requests a subset of features from the
backend and uses it to draw. A map tool sends off another
tool-specific request to get back the desired subset of features it
needs. An attribute table display again sends off a new request each
time it's opened. Some providers (eg Arcgis Feature Server provider,
WFS provider) locally cache and renew the features, but others get
them anew with each new request*. There's no master/"root" list of
features associated with a layer.

(* a generic local cache of features is highly desirable and would
really boost QGIS performance, but it's not a trivial task, and to
quote a famous guy: "the laborers are few".)

> Because these bugs suggest an implementation that either creates a second, filtered list of features and the canvas draws gets passed that list instead of the "root" list while the tools still work off of the "root" list, or that the canvas drawing code was modified to to do the temporal filtering itself and draw only a subset of the "root" list passed to it.

See above -- the map tools indeed send off their own request for the
features they want. Some of the tools restrict the request to features
visible in canvas, others don't. Some ignore the canvas filtering by
design, others don't and should be investigated accordingly.

> You’ve said that you had to fix the bugs I reported separately, but why? To my mind, the NTC should be filtering that "root" list of features, and then nothing else needs to be modified.

Again, there's no such list to draw on.

> Not the code that calls the canvas drawer method, not the code for the selection tool. Not the code of the identify feature tool. Don’t change the tools to be time-aware individually, keep them time-agnostic and change the dataset that gets passed to them, and implement that in a centralized location. Then later when somebody with that rare use case of needing to select even the filtered-out features asks for it, you can modify that centralized location to return the full dataset, not the temporally filtered one if certain conditions are met, like an app setting or using the Alt key when clicking on the canvas, or whatever, and it will work for all tools, not just one.

Yes, this would be a desirable refactoring of the canvas and map tools
class, e.g. to add a getVisibleFeatureRequest() method to
QgsMapCanvas, and then call this from the map tools and refine as
necessary. A lot of QGIS developers do this kind of refactoring and
code clean up on a nearly daily basis... and don't get paid for it.
Yep, that sucks. Yep, it's totally unfair. To quote another famous
guy: "such is life".

I'm not interested in doing this refactoring at all...  My motivation
to work on map tools and temporal handling is 0. I've used up all my
emotional energy in that sphere on this email thread. That said, I'll
gladly review a pull request doing this refactoring if you want to get
involved and constructively contribute back to the project.

> (I’m willing to bet you a $100 donation that usage cases that need the selection tool to work on all the features including those filtered out by the NTC and not drawn is tiny to nonexistent, compared to the one where the tool is expected to work in a WYSIWYG manner only on the visible features. If I am wrong, I'll double that donation.)

Why not just take your $$$ and directly fund (partially/fully ??) the
fixes you want. That's the MOST effective way to get things
added/fixed in QGIS. (If you don't like that, blame Adam Smith...)

Nyall


>
>
> However, since I do not know the codebase, I asked for that help in understanding how the NTC was implemented and why that way, due to other considerations of which I would be unaware that prevented that. It was also why I phrased my final line of criticism as “I feel” rather than as a statement of fact.
>
Nyall

>
>
> The NTC affects what gets displayed on the map canvas, therefore other tools that operate on features on the map canvas should have been tested to see how the interacted with the NTC. "Does my new capability hide features from the map canvas? I should test the tools that one can use on features on the the map canvas to make sure it doesn't operate on the features hidden by my new capability.” Do you see how it looks like that type of testing was not done before the branch with the NTC got merged with master?
>
>
> On Tue, Jan 5, 2021 at 5:00 AM Nyall Dawson <nyall.dawson at gmail.com> wrote:
>>
>> On Tue, 5 Jan 2021 at 05:42, Cory Albrecht <maps at hanfastolfe.com> wrote:
>> >
>> > > Please be very wary of your language in future -- every piece of feedback worded like this directly equates to a developer losing interest in volunteering their time on QGIS, to the harm of all.
>> >
>> > I hear what you're saying, Nyall, I apologise if it sounded harsh, but what do you want me to say after I find out that the latest of several bugs has compromised my data and now I have to hunt down an unknown number of duplicates silently created over the last few months of work? If you empathise, do you understand how all these bugs and my data being compromised might make me feel that this feature was not done very well and that there was a lack of adequate testing?
>> >
>> > > As background, I implemented time handling for vector layers as a VOLUNTEER (completely unpaid). Would you have preferred I didn't do this, and we had no time support for vectors at all?
>> >
>> > As to what I would have preferred, well, I've reverted back to 3.10 to use the old TimeManager plugin to avoid using the NTC. I would have preferred that the feature branch you used to implement the NTC had not gotten merged and the feature included in release versions until it had been checked to work with and not break basic features like the selection tool. So now I have to work under some Damoclean time limit for a couple of months until 3.16 replaces 3.10 and I will have no choice but to move to a version with the NTC and these bugs.
>>
>> Full disclosure: I don't even really consider these issues as bugs.
>> Functionality gaps, yes, but it's important to keep in mind that the
>> new temporal framework was designed primarily around visualisation of
>> time based data, and that using it for data analysis has been a
>> secondary goal which has been slowly chipped away at since the initial
>> implementation. The selection tool was NEVER broken, it was just never
>> upgraded to be time aware. There's plenty of features in  QGIS which
>> don't completely work alongside each other, and these are not always
>> bugs but often are feature requests.
>>
>> I'd rather shelve that part of the discussion now, because I suspect
>> we'll just keep going around in circles here at increasing levels of
>> emotion, and I don't think that's healthy. I realise there's hurt
>> here, but I DO think you are a valuable member of this community and
>> I've much appreciated your previous diligence with submitting quality
>> tickets. I'd rather move forward and find a way that we can move past
>> this.
>>
>> Which leads me to a question: what exactly do you think should have
>> been done differently here? The way I see it:
>>
>> 1. The temporal framework was in discussion and consultation phases
>> for years prior to implementation. There were public calls for
>> comments on the related QEPs, and consultation with all relevant
>> stakeholders, including representatives from Kartoza (WMS-T), Lutra
>> (mesh temporal handling), and the Time Manager plugin maintainers.  It
>> was a group effort which pulled in ALL the expertise and experience of
>> the QGIS project community.
>>
>> 2. QGIS already has some of the most stringent and rigorous code
>> review processes around, and the pull requests implementing the
>> temporal framework were subject to all these processes and peer
>> review. There's plenty of developers who would attest to how difficult
>> it is to get code merged into QGIS, due to the very strict coding
>> guidelines and processes which govern the codebase. I fail to see
>> anyway we could realistically further tighten these code review
>> requirements without completely strangling the development of QGIS.
>>
>> 3. If you're asking for the pace of feature development to be slowed
>> then that problem was addressed years ago when the LTR releases were
>> introduced. The QGIS website is quite clear in advertising the LTR as
>> the officially recommended version for stability and for mission
>> critical work, while the non-LTR releases are clearly branded as
>> "bleeding edge". I'd make the case that in the situation you've
>> described you should never have been using the non-LTR release for
>> this work without doing your full diligence to determine that it
>> worked correctly in your workflow. And guess what? Most of the bugs
>> are now fixed in time for 3.16.4, when 3.16 officially becomes the
>> next LTR release! Again, I can't see how the project can do anything
>> differently here. In fact, the next LTR (3.16) will even be supported
>> for a massive 2 year period to ensure even more stable releases are
>> available for mission critical work.
>>
>> So I ask again, in the full spirit of reconciliation and moving
>> forward: what concrete, practical changes do YOU think the project
>> should make?
>>
>> Nyall
>>
>>
>>
>> >
>> > On Sun, Jan 3, 2021 at 9:32 PM Nyall Dawson <nyall.dawson at gmail.com> wrote:
>> >>
>> >> On Sat, 2 Jan 2021 at 10:23, Cory Albrecht <maps at hanfastolfe.com> wrote:
>> >> >
>> >> > Can somebody help me under the basics of how things work inside QGIS starting from when it loads all the features for a layer through the steps, and then finally drawing them on the map canvas, specifically with respect to the new temporal controller (NTC)?
>> >> >
>> >> > The issues caused by the NTC have been very frustrating for me as I make mostly (historical) timeline maps and I relied heavily on the old TimeManager (OTM) plugin by Antia Graser and group. So many tasks are now much more laborious or difficult because so many tools are just not time-aware.
>> >> >
>> >> > Was it not possible to add the NTC in such a way that would have still let all the other features work as before with the filtered feature set before being made time-aware, rather than confusingly operating on the unfiltered set? Or perhaps it shouldn't have been turned on until the infrastructure was there for the tools to be time-aware right away?
>> >> >
>> >> > Because I've just submitted yet another bug about the NTC, this time for the selection tool, and it has me more than a little annoyed. As a result of this bug I now have an unknown number of duplicate objects in multiple layers across multiple databases/projects that I unknowingly pasted into them over the past several months since the NTC was added to QGIS.
>> >> >
>> >> > I feel that the NTC was both poorly thought out and badly implemented as all these bugs would indicate.
>> >>
>> >> I can empathise with your frustration, but this is a very complex discussion!
>> >>
>> >> As background, I implemented time handling for vector layers as a
>> >> VOLUNTEER (completely unpaid). Would you have preferred I didn't do
>> >> this, and we had no time support for vectors at all? Please be very
>> >> wary of your language in future -- every piece of feedback worded like
>> >> this directly equates to a developer losing interest in volunteering
>> >> their time on QGIS, to the harm of all.
>> >>
>> >> I've fixed two of your issues here
>> >> https://github.com/qgis/QGIS/pull/40834, as an UNPAID VOLUNTEER.
>> >> Without funding I'm just not interested in fixing the snapping related
>> >> issues. I don't personally have any need for these, and the QGIS
>> >> snapping code isn't something I'm motivated in getting involved with
>> >> at all. Perhaps there's another developer interested in looking at
>> >> this, or perhaps a developer will take a look at this during the 3.18
>> >> bug sprint. Or maybe someone will pay for this fix and financial gain
>> >> will be the motivation.
>> >>
>> >> I like making the world a better place through volunteering my time on
>> >> making a first-class desktop mapping application, but I'm far from
>> >> being a slave to this, and my family, garden, and drum kit want my
>> >> time too!
>> >>
>> >> Nyall
>> >>
>> >>
>> >>
>> >> > _______________________________________________
>> >> > QGIS-Developer mailing list
>> >> > QGIS-Developer at lists.osgeo.org
>> >> > List info: https://lists.osgeo.org/mailman/listinfo/qgis-developer
>> >> > Unsubscribe: https://lists.osgeo.org/mailman/listinfo/qgis-developer