Bug with TreeView in JUCE 6.1.2 (performance)

We recently updated from JUCE 6.0.7 to JUCE 6.1.2. After the upgrade we noticed, that our TreeViews suddenly had very bad performance, when adding new nodes.
Things that took only a split second with JUCE 6.0.7, suddenly took several seconds in JUCE 6.1.2.

Our scenario is the following:
We have a TreeView, with one root node, which contains a LOT of children (approximately 1000 children). And the children are added to that that tree during run time (while the TreeView is already open and visible).

I debugged the problem and could see that TreeViewItem::getItemHeight() was called a lot of times in JUCE 6.1.2. Much more often than in JUCE 6.0.7.
If you add 1000 nodes to such a TreeView in JUCE 6.1.2 TreeViewItem::getItemHeight() is called a whopping 502502 times! In contrast: doing the same in JUCE 6.0.7 TreeViewItem::getItemHeight() is called only 1002 times. In JUCE 6.0.7, the number of calls to TreeViewItem::getItemHeight() correspond linearly to the number of added nodes. But in JUCE 6.1.2 its more like a square-law relation (I suspect it is node * (nodes+1) / 2).

I tracked it down to a change in TreeViewItem::treeHasChanged().
In JUCE 6.0.7 TreeViewItem::treeHasChanged() calls ownerView->itemsChanged(). But in JUCE 6.1.2 it calls ownerView->updateVisibleItems() instead. The problem is, that ownerView->updateVisibleItems() in turn calls TreeView::updatePositions(), which iterates through all existing items and calls TreeViewItem::getItemHeight() on them. So basically, adding only a single new item to the TreeView, makes the TreeView update all items in JUCE 6.1.2.

I have attached an example project, which you can use to test the behaviour (See TreeViewPerformanceTest.zip).
Here is how to reproduce:

  1. Run the test project.
  2. Click on the “Create Tree” button.
  3. Check what the label "calls to getItemHeight = " says. If you run it with JUCE 6.0.7, its going to show 1002 calls. But if you run it with JUCE 6.1.2 its going to show 502502 calls.

I assume that the changes to the TreeView have to do with the new Accessibility feature?
Would be great if the TreeView could get back the nice responsiveness it had in JUCE 6.0.7.

TreeViewPerformanceTest.zip (15.6 KB)

3 Likes

Thanks for the detailed report and example! That does indeed seem to be a regression with the TreeView changes that I made for adding accessibility support. I’ve pushed a fix to the develop branch which should improve things, but please let me know if you are still experiencing performance issues:

I’m afraid this broke scrollToKeepItemVisible (TreeViewItem* item); It doesn’t seem to work anymore.

Also, you migth want to check the value used in addOffscreenItemBuffer (item, 2, false); in lines 607 and 618. Dragging-to-scroll and leaving the area of the viewport causes a crash on my system (it previously caused the tree to jump back to the position it had at the beginning of the drag gesture, also unwanted behaviour). Increasing the value from 2 to around 25 fixed the issue for me.

I’m not seeing this using the tip of develop. I’ve modified the ValueTreesDemo to add the following line to the constructor -

tree.scrollToKeepItemVisible (tree.getItemOnRow (tree.getNumRowsInTree() - 1));

and it scrolls to the correct item.

I’m not seeing this behaviour either. I’m able to drag the tree items in the demo out of the viewport and it behaves normally. Can you provide some example code that reproduces the issue? If the async updater changes are causing issues then the following patch might resolve them, can you try applying it locally and see if it fixes the crashes you are seeing?

0001-TreeView.patch (5.3 KB)

I did not mean dragging items out of the viewport but rather scrolling the viewport by dragging and then continuing the dragging gesture while leaving the viewport. This is probably only relevant for mobile devices. If you run the Demorunner on an iOS simulator, and on the ValueTrees demo you drag on the left side of the items slowly upwards to scroll the viewport, shortly after leaving the viewport area the view suddenly “jumps” back to the initial position the viewport had before the scrolling took place. (Note: make sure to drag slowly). This is problematic if on a mobile device one has a small viewport which one wants the user to scroll by dragging.

As soon as I have time I will investigate what’s leading to the crash and if the patch works. Thanks for your help!

Hi Ed,

thanks for the quick fix :slight_smile:
Gonna test it soon and give you feedback.

ScrollToKeepItemVisible is indeed working fine, no idea what caused it to temporarily stop working as I switched to the develop branch, sorry for that.

The patch didn’t solve the other problem. What I am seeing is that juce_Component.cpp line 256: getDesktopScaleFactor() causes an EXC_BAD_ACCESS. But even when that does not happen (as in the Demorunner), the view “jumps”. All is fine, though, with a larger value in addOffscreenItemBuffer.

I’ve pushed a fix for these issues to the develop branch:

Please give it a try and let me know if you’re still experiencing problems.

Thanks for working on this, but it doesn’t solve the issue, at least for me. I am using a TreeView as a menu on mobile devices, so I guess that my case is somewhat special.

You can see what the problem is through some minor changes to the ValueTreesDemo:

Within resized() and before tree.setBounds (r), add:

r.removeFromBottom (140);
r.removeFromTop (140);

And do not implement the getDragSourceDescription() method.

Now, if you drag the viewport to scroll it, shortly after leaving the viewport area, the view “jumps” back to its initial state and you cannot keep scrolling. That’s the issue I described above which I am still seeing and which goes away with something like addOffscreenItemBuffer (item, 25, false);

But as I said my case is probably a bit special. I also had to make some changes to the mouseDownInternal and mouseUpInternal methods so that the user can drag-to-scroll without selecting items and selecting items occurs only by actually clicking, but that’s another topic…

Thanks, can you see if the following commit fixes the issues you are seeing?

1 Like

Yes, that works perfectly. Thanks!

1 Like

Hrm… I’m still not sure scrollToMakeItemVisible works entirely as expected in 6.1.5(6?). Somewhere recently (since 6.0.7?), updating the item positions was moved to async call updateVisibleItems:

void TreeView::updateVisibleItems() { viewport->recalculatePositions (TreeViewport::Async::yes); }

Which, apparently, updates the positions of the items currently ‘visible’ in the viewport… but forgetting those currently ‘invisible’ beyond the bounds of the viewport? So that when scrollToMakeItemVisible is called;

void TreeView::scrollToKeepItemVisible (TreeViewItem* item)
{
    if (item != nullptr && item->ownerView == this)
    {
        updateVisibleItems(); // called asynchronously, so will have no effect on subsequent retrieval  (below) of the items current .y position regardless of whether or not it is currently visible in the viewport.

        item = item->getDeepestOpenParentItem();

        auto y = item->y;  // this will still be zero for any newly added items which have not yet had their positions calculated, and for items outside the viewport(?)
        auto viewTop = viewport->getViewPositionY();

// and so, the following will always fail in this case; y = 0
        if (y < viewTop)
        {
            viewport->setViewPosition (viewport->getViewPositionX(), y);
        }
        else if (y + item->itemHeight > viewTop + viewport->getViewHeight())
        {
            viewport->setViewPosition (viewport->getViewPositionX(),
                                       (y + item->itemHeight) - viewport->getViewHeight());
        }
    }
}

Maybe the code assumes that any newly created items are always created inside the viewport? But in this case even visible items won’t have the updated positions yet by the time setViewPosition is called. So then what’s the point of ‘scrollToMakeItemVisible’?.. as the name implies; “please find the item currently invisible beyond the top or bottom of the viewport, and scroll until it becomes visible”… and currently it’s not doing that.

I suppose there are a number of ways to fix this, but AFAICT they would all involve changes to JUCE directly:

  • Syncronously call some version of update_All_ItemPositions in the case of scrollToMakeItemVisible
  • Perhaps the position can be calculated easily enough (from parent, etc.) when items are added/initialized in addSubItem (currently the position are not initialized there)
  • provide a method (or access to existing method via the TreeView or TreeViewport) that lets a subclass of TreeView and/or TreeViewItem force the item positions to be refreshed syncronously before attempting to scrollToMakeItemVisible

Interesting.
I am wondering if what you describe might also have to do with another bug report of mine?
Where it does not correctly restore the scroll position of a TreeView from TreeView::restoreOpennessState() :

Probably… seems to affect anything that uses scrollToMakeItemVisible that doesn’t assume all the items are visible and the tree hasn’t changed since setRoot was called. For example our app reads an app setting at startup that restores the id of the last selected valueTree node and attempts to scroll to and select it on startup so the user can resume where they left off… this (among other case / scenarios) no longer works with the current TreeView.

bump perhaps questionable etiquette, but since we need to release a fix for this, it would help to know if there are any plans for the juce team to fix this(?) …easy fix, but prefer to avoid modifying juce when possible. In hopes of simplifying future merges from juce trunk, I offer simple solution:

void updateVisibleItems(Async useAsyncUpdate = Async::yes);

...

void TreeView::updateVisibleItems(Async useAsyncUpdate) { 
    viewport->recalculatePositions (useAsyncUpdate); 
}

...
void TreeView::scrollToKeepItemVisible (TreeViewItem* item)
{
    if (item != nullptr && item->ownerView == this)
    {
        updateVisibleItems(Async::no); // just say no (in this scenario)

        item = item->getDeepestOpenParentItem();

        auto y = item->y;
        auto viewTop = viewport->getViewPositionY();

        if (y < viewTop)
        {
            viewport->setViewPosition (viewport->getViewPositionX(), y);
        }
        else if (y + item->itemHeight > viewTop + viewport->getViewHeight())
        {
            viewport->setViewPosition (viewport->getViewPositionX(),
                                       (y + item->itemHeight) - viewport->getViewHeight());
        }
    }
}

I can imagine other scenarios where the positions might need to be updated synchronously, perhaps by simply providing public access to that update method, but this fixes the bug that was introduced from 6.0.7 to 6.1.6(?) at least ,and works for my purposes anyway.