yotamberk / timeline-plus

Timeline - chronological visualization of your data
MIT License
137 stars 36 forks source link

Range items overflowing the timeline start and end dates are initially hidden #139

Open taavi-halkola opened 5 years ago

taavi-halkola commented 5 years ago

Hi,

There seems to be a problem with range and background items where they are not initially visible in the timeline if the item(s) start and end range overflows the timelines visible window.

For example having a range item starting from 2019-03-15 to 2019-03-21 is not visible in a timeline initiated with start date of 2019-03-17 and end date 2019-03-19. Once the window is scrolled enough to the future and the item becomes visible it will then stay visible even when scrolling back to the said dates of 2019-03-17 to 2019-03-19.

The hidden items are also not found with timeline.getVisibleItems()

I can see how this could possibly cause long events to never be seen. Any idea what might be causing this?

taavi-halkola commented 5 years ago

Did some digging and it seems to be caused by a block of code in Group.js which was added back after being removed from vis.js and being discussed in #81 :

if (!this.isVisible && this.groupId != "__background__") {
    for (var i = 0; i < oldVisibleItems.length; i++) {
        var item = oldVisibleItems[i];
        if (item.displayed) item.hide();
     }
     return visibleItems;
}

Removing this block seems to remove the problem but performance suffers greatly. Any ideas?

It was mentioned to have been removed because of an issue with an old vis.js version. What issue might that have been? And what was the different fix for that issue in timeline-plus?

taavi-halkola commented 5 years ago

Here is something to help out if anyone is looking into this. The problem appears to be the item.hide(); call hiding all previously visible items without checking if they actually should be hidden. Causing the next call to Group._updateItemsInRange() to completely miss these items. Replacing

var item = oldVisibleItems[i];
 if (item.displayed) item.hide();

with

this._checkIfVisibleWithReference(oldVisibleItems[i], visibleItems, visibleItemsLookup, range);

seems to do the trick and scrolling performance with hidden groups still seems much better than without the whole block.

sbusch commented 5 years ago

I have the same problem but the block @taavi-halkola mentioned is IMO unrelated to the problem of such items not appearing (it's about hiding all items of invisible groups). Removing the block did not solve the problem.

I'm currently working on a fix.

jaytrepka commented 5 years ago

Hi, is there any progress here or is there something i can help with? I really need this.

sbusch commented 5 years ago

@jaytrepka I have a working fix, but with the downside of losing some optimisations regarding large item sets. That's why I didn't push a PR yet...

I'm considering to start such a PR but marked as "not ready to be merged" to start a discussion how this could be merged by @yotamberk. I hope I can continue to work on it soon, maybe in the next few days.

taavi-halkola commented 5 years ago

@sbusch I've come to the same conclusion. There are two occasions where this is affected and both seem to affect performance...

@jaytrepka There seems to be a possible workaround but i am not sure what negative side effects it may or may not have. When you are initializing your timeline start with empty groups and items. Then first add items through timeline.setItems() and finally the groups with timeline.setGroups(). Also i do not know if this also has the negative effects of losing performance or not. You should probably test that first if running a performance critical application.

timeline = new Timeline(container, [], [], options)
timeline.setItems(dataset)
timeline.setGroups(groups)

The main difference here is when initializing the timeline with groups and items the order is the other way around and for some reason this makes the difference. Although drawing the groups first is the correct way to go and commented as important in Timeline.js.

// IMPORTANT: THIS HAPPENS BEFORE SET ITEMS!
if (groups) {
    this.setGroups(groups);
}
// create itemset
if (items) {
    this.setItems(items);
}
sbusch commented 5 years ago

Et voila, here's the draft PR #157

Appreciating any comments

sbusch commented 5 years ago

@jaytrepka If you're making your own builds, you can safely merge this PR into your fork. The scope of changes is quite small and besides the performance drawback it should just work.