Skip to content
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

Wait for tracks to be fully drawn before returning to caller #1844

Closed

Conversation

eternal-flame-AD
Copy link
Contributor

Nice project! Thanks for sharing this (⌒‿⌒)

This PR fixes two things (the 2nd fix depends on the 1st fix, I can open a new PR if we decide to take the middleground).

The first is a regression on TrackView#renderSVGContext which errors out because of a missing SampleInfo#isInitialized(), I implemented it.

The second one is a more aggressive change. I will explain my reasoning below:

Problem

I wanted to write a pipeline to automatically generate SVG without human intervention. However the current implementation does not wait for the tracks to be loaded before returning to the caller, so I have no reliable way to tell when should I take the SVG.

Fix

I added await's to the updateViews() call. Caller can decide whether they want blocking behavior by awaiting or floating the API Promise as a whole, I don't think we need this level of granularity because in this (load -> draw -> display) sequence, as a user I cannot think of a reason I have to know when is only the first step done.

I also removed the undocumented sync option, it is not used or mentioned anywhere and the current infrastructure made it impossible to change for ex. the ruler to sync. If we really want this level of customization I think an async option is better, by default we wait for the updates on these tracks to be done before resolving (which I think is a more natural behavior by common sense: an API that returns a Promise I would expect it to be done with everything before telling me "I'm done!" unless I say otherwise), and advanced users can selectively make some tracks update untracked (for perf reasons?).

Demo:
Unpatched (built from 1e97a6d) <- This produces partial SVGs & unpredictable.
Patched <- This produces full SVG

@jrobinso
Copy link
Contributor

Won't this change force serial loading of tracks, when multiple tracks are loaded simultaneously? The call to updateViews() often resulting in loading additional data. Although javascript is single threaded the loads happen simultaneously via xhr requests. From my recollection of earlier testing its a major performance hit to "await" there when many tracks are loaded, e.g. 50 bigwig tracks.

@jrobinso
Copy link
Contributor

If you wish please submit a separate PR for the sample info bug, if not I can copy & paste the fix. I think you can achieve what you need by keeping the "config.sync" option and using it, and additionally wrapping the update for the ruler track update (rtv.updateViews()).

@eternal-flame-AD
Copy link
Contributor Author

eternal-flame-AD commented Jul 11, 2024

@jrobinso Sure I will cherry pick into a new PR. I understand your concerns I will think about it more and if I found a solution I will open a new issue/PR for that. In the meantime I can keep an off tree patch for my issue.

@jrobinso
Copy link
Contributor

One solution might be to "await" when loadTracks is called from the API, but don't await when called internally. This might be a good long term solution. In the meantime extending and using "config.sync" should work for your situation with minimal disruption. In any event the 2 issues should be distinct PRs.

Thanks for your contributions!

jrobinso added a commit that referenced this pull request Jul 12, 2024
…s for API users. These updates are fast, and the await is harmless. See PR #1844
@jrobinso
Copy link
Contributor

I've added an "await" to the ruler & ideogram updates. I think you can accomplish what you are asking for by setting config.sync to true. This is actually the purpose of this property, from a different but similar use case.

@jrobinso jrobinso closed this Jul 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants