daattali / timevis

📅 Create interactive timeline visualizations in R
http://daattali.com/shiny/timevis-demo/
Other
655 stars 157 forks source link

Upgrade to latest visjs Timeline #50

Closed daattali closed 3 years ago

daattali commented 6 years ago

Currently timevis uses visjs v4.16.1

It'd be nice to update to the most up to date version because they have new features. But there are some issues (and perhaps more, everything needs to be tested thoroughly to make sure nothing breaks):

knbknb commented 6 years ago

I have simply replaced timelines_files/vis-4.16.1/vis.min.js and timelines_files/vis-4.16.1/vis.min.css with those versions from timevis-4.21.zip.

It works for me.

Why did I update? Because v4.21 also has more events defined. I need the new onInitialDrawComplete Event which is not available in v4.16. (I don't know when it was added. I didn't find a mention of this event in the visjs release notes)

Just saying, maybe it helps someone who thinks about updating.

daattali commented 6 years ago

Yes it does work. However, there are several non-minor regression bugs in the new visjs timeline version, as I outline in the issue description. Until those are resolved, I would prefer to hold off on updating because these issues mean that many people who will upgrade to the new timevis version will have bugs.

visjs is really great but it's a bit of a shame that bugs don't get addressed and that documentation of new features is very lacking (I also ran into that problem of not knowing when new features got added because they aren't documented)

daattali commented 5 years ago

Merge #70 after upgrading

strazto commented 5 years ago

Are you planning on migrating to the timeline-plus project, when you do upgrade @daattali ?

I experimented with upgrading to the library myself, and found that it was doable but the maintainer's choice of object name (timeline) collided with timevis's timeline.

It's actively maintained, and seems a bit less bloated than the vis.js distro.

I use timevis for work and I find it really really useful, so if you're keen to see some fixes on timeline-plus before you're comfortable upgrading, let me know

daattali commented 5 years ago

I will upgrade to whichever one, as long as the regression bugs I mentioned are fixed. I opened the same issues on timeline-plus repo, if they are fixed over there then yes I will look into switching. But I will not upgrade until the regression bugs are fixed.

daattali commented 4 years ago

Update: I again looked into updating to the latest version, but unfortunately it seems like the bugs I reported to visjs Timeline a few years ago are still unresolved and I cannot update until they are fixed.

strazto commented 3 years ago

So the bugs you've reported have open PRs & will be resolved soon enough.

Personally, I tested the head of the visjs/vis-timeline repo against one of my branches, & I did find that it was hanging my brower.

I'm not sure what's wrong now, but I'm going to do some work to ease the migration (besides the bugfixes I've submitted), because overall the quality of vis-timeline HAS improved, and it has got more features

daattali commented 3 years ago

Interesting. Please do report back with any details (also, what do you mean by "hangs"? Is everything just a bit slower, or does it actually freeze? Or any input, or some specific compelx cases?)

strazto commented 3 years ago

I'm not so sure it's an update problem (I think the branch I'm running has slightly older version of vis) - my particular dataset seems to be causing the hang, it eventualyl does render and runs just fine, but before that it seems to chug for >2 minutes.

Profiling it tells me it's spending a LOT of time running _updateSubgroupSizes .

image

I'm checking out my dataset, it's possibly bigger than I expected, & maybe I'm specifying subgroups wrong & it's blowing out on a O(n^something) loop?

strazto commented 3 years ago

It's not a problem with my subgroups, for whatever reason, for my dataset it seemed to trigger like 40000 _removeFromSubgroup calls

daattali commented 3 years ago

Have you tested vs the javascript library , using purely just html+JS? That would tell us if its a timevis problem or if it's a problem with visjs which means you can stop investigating and pass it along to them

strazto commented 3 years ago

Oh, sorry to put you through the effort of trying to help me debug (I forgot to reply with my findings) this was a @matthewstrasiotto problem, I forgot to filter one of the inputs to the dataset, a bunch of items in nested groups with no corresponding parents.

The items weren't rendering because they didn't have valid group hierachies, but vis-timeline had to do a LOT of work before finally rendering

D3SL commented 3 years ago

Are these same few issues still holding back updating?

daattali commented 3 years ago

Yes. I used to check every month consistently but I don't recall the last time I tried. You (or anyone else) is welcome to try to see if they're fixed :)

strazto commented 3 years ago

IIRC I fixed a majority of the issues in vis.js . I don't remember what was left. Personally I maintain a fork of timevis that uses the updated library, since I'd dealt with at least the problems i cared about https://github.com/matthewstrasiotto/timevis

daattali commented 3 years ago

@matthewstrasiotto you have an open PR https://github.com/daattali/timevis/pull/77/files but as far as I can tell that PR doesn't include updating the version, I'd love a PR if you think the issues can be fixed

ismirsehregal commented 3 years ago

Just to add another reason for updating:

With the current CRAN version the initial rendering of a timevis object is very slow if many items / groups are provided.

I tried using a start and end time in the options as suggested here - but it doesn't help.

It seems some relevant updates were included by now.

library(shiny)
library(timevis)

myItems <- seq_len(1500)

data <- data.frame(
  id = myItems,
  start = Sys.time()-myItems,
  content = paste0("Item_", myItems)
)

ui <- fluidPage(
  timevisOutput("myTimevis")
)

server <- function(input, output, session) {
  output$myTimevis <- renderTimevis(
    timevis(
      data,
      options = list(start = data$start[nrow(data)], end = data$start[nrow(data)-10])
    )
  )
}

shinyApp(ui, server)
daattali commented 3 years ago

There are certainly many good reasons to update! And I would love to. But only once the regression issues are fixed.

ismirsehregal commented 3 years ago

The above makes your package unuseable regarding my usecase. What a pity - it looked really promising but cosidering the age of those regression issues it seems I'm flogging a dead horse. Will switch to plotly gantt charts then.

D3SL commented 3 years ago

The above makes your package unuseable regarding my usecase. What a pity - it looked really promising but cosidering the age of those regression issues it seems I'm flogging a dead horse. Will switch to plotly gantt charts then.

Not just yours, that's a crippling performance issue in general. To be honest I think the "regressions" range from trifling quirks to a minor annoyance at most while refusing to update on the other hand has led to profoundly crippling the R version by missing out on significant feature advancements, as well as severe performance issues like you detail.

But it's Daattali's package and his call in the end.

daattali commented 3 years ago

I think a happy middleground would be to introduce a new function timevis2() that can use the updated library. I'll try that some time, or anyone else is welcomed to help with a PR. As little code as possible should be affected.

daattali commented 3 years ago

Took me an entire weekend because of a lot of non backwards compatible changes... but this is done 97fbd7bf566ff6605e18035c1c8ac8b52af181e7

timevis() has been upgraded to visjs 7.4.9. Feel free to test and let me know if there are any regression issues

ismirsehregal commented 3 years ago

Thanks for your work! Highly appreciated! I'll let you know as soon as I've tested the latest version.

ismirsehregal commented 3 years ago

It seems with te new version, the start and end options I'm setting in my above example are no longer taken into account.

daattali commented 3 years ago

Did that used to work previously, is it a regression bug? When I try that I see nothing show up - I think it's too many items. Anyway if you think it's a real bug, please open an issue