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

refactor tier data loading to support custom renders. #148 #154

Merged
merged 2 commits into from
Apr 22, 2015

Conversation

Traksewt
Copy link

allows for the tier data to be retrieved without rendering the default tiers. The default tiers can be rendered when needed by calling cbrowser.refresh(), which will use the cached data. updating the status and drawing the tier is now part of the default tier render method. Updating the tier height incase of exception was not needed as it was happening at the end of draw()->paint().

@dasmoth
Copy link
Owner

dasmoth commented Apr 22, 2015

Looks okay except that the call to _notifierToStatus has been removed -- this will break visible error reporting (which I know has a pretty odd data-flow at the moment, but does work). Would it cause you any problems if I reinstated this when I merge?

Or do you need the tiers to be completely "headless"?

@Traksewt
Copy link
Author

Notifytostatus should get called by the default tier renderer when it calls
updateStatus. That way you can do what you like with the status for custom
renderers.
On 22/04/2015 4:27 PM, "Thomas Down" notifications@github.com wrote:

Looks okay except that the call to _notifierToStatus has been removed
-- this will break visible error reporting (which I know has a pretty odd
data-flow at the moment, but does work). Would it cause you any problems if
I reinstated this when I merge?

Or do you need the tiers to be completely "headless"?


Reply to this email directly or view it on GitHub
#154 (comment).

@dasmoth dasmoth merged commit b80e96f into dasmoth:master Apr 22, 2015
@dasmoth
Copy link
Owner

dasmoth commented Apr 22, 2015

Okay, understand now -- makes sense.

Medium-term, I wonder if it might make more sense to start allowing subclassing of the DasTier prototype (which probably shouldn't be called that any more...)

@Traksewt
Copy link
Author

Thanks. I started a branch where I split Tier class into Tier (for the
data) and TierRenderer for the GUI parts. This is to support multiple
renderers from the one tier. I don't want to replace the linear dalliance
render, just add a new visualisation option.

I had a list of TierRenderers in the cbrowser.

It got tricky splitting the tier config as some of the config was for the
data, some for the style. Some style items I'd be happy to be consistent
(i.e. available in Tier), like feature colour. So I put it on the
backburner, and focused on the data retrieval refactor.

Even if we go down this path and allow for multiple TierRenderers to listen
off one Tier (data update), we would still have the case where we don't
want all TierRenderers to render at once (e.g. if one is offscreen in a
tabbed pane), so we'd need flexibility on selecting which TierRenderers to
trigger off a particular data retrieval.

On Thu, Apr 23, 2015 at 5:14 AM, Thomas Down notifications@github.com
wrote:

Okay, understand now -- makes sense.

Medium-term, I wonder if it might make more sense to start allowing
subclassing of the DasTier prototype (which probably shouldn't be called
that any more...)


Reply to this email directly or view it on GitHub
#154 (comment).

@dasmoth
Copy link
Owner

dasmoth commented Apr 23, 2015

I don't think I like the idea of multiple TierRendererers per Tier. There's already a caching system in place which means that if two Tiers have the same source configuration (see sourceConfigsAreEqual etc.), they'll be backed by a single data fetcher (with appropriate routing of data), so in general I think having extra Tier objects is probably the right answer. We might need to allow tiers to be "headless" (e.g. don't have a viewport in the main browser component).

Having different sets of tiers fetching at different times becomes trickier... but here's a thought: what if we allowed multiple KnownSpace objects, potentially associated with different sets of tiers. Each distinct view has its own KnownSpace. That was actually the original plan when I introduced KnownSpaces -- I've also had an eye on supporting split-screen viewers (e.g. see two interacting genomic regions side by side), which would be two KnownSpaces with (usually) the same set of Tiers.

This does get kind-of complicated, though.

@Traksewt
Copy link
Author

Does the single data fetcher allow for multiple tiers retrieving data from the same source but looking at different locations, to handle requesting for both locations at once? If so, that is great, it would be useful for my other scenarios.

In my case at the moment, I want the tiers with different rendering to be in sync, as I am showing the same data, just in a different way. So they can share the knownspace (which will happen anyway) and all data retrieved, it just needs to render it differently. Hence ideally I'll just register the callbacks once, as I care for when the data is loaded/changed, not when it is visualised.

Parts of your solution is common to mine, as making tier headless was what I was trying. If it is headless, then the viewport needs to go somewhere, which sounds like it would attach to your view idea? Would the default tier rendering be a subclass of the basic Tier, or does it remain as it is, and I overwrite the rendering methods for my custom rendering in a subclass? Either way, in this design each of the tiers would hold a reference to the features/sequence even if they are the same (though using the same knownspace they will both point to the same feature array). And they would have separate listener lists.

This can work, just a design decision on whether to separate model/view (and refactor the rendering to use the view) or subclass. I'm easy either way.

One thing I'm not clear on, is how does the KnownSpace know to use the same featureCache for different tiers with the same config? It looks like it is keyed by the tier object itself. I would have thought that featureCache would need to be keyed by a hash (or unique name) of the dasSource.
kspace.js ln 241
var baton = thisB.featureCache[tier];

I can't see reference to sourcesAreEqual in kspace. Thanks.

@dasmoth
Copy link
Owner

dasmoth commented Apr 24, 2015

Yes, multiple KnownSpaces sharing fetchers should be absolutely fine.

The featureCache in KnownSpace is separate from the layer which routes data from a single fetcher to multiple tiers (see CachingFeatureSource and the top of sourceadapters.js). In some senses, the cache in KnownSpace might be a little wasteful (and I have, on occasions, pondered whether there might be ways to get rid of it entirely), but since -- in the two-tiers-share-single-fetcher case -- they'll both be getting the same set of objects back, it's not actually wasting much memory.

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