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

Layout of track heights is inconsistent #1190

Closed
cmdcolin opened this Issue Aug 22, 2018 · 10 comments

Comments

Projects
None yet
3 participants
@cmdcolin
Copy link
Contributor

cmdcolin commented Aug 22, 2018

It was reported in #1187 that the track height can sometimes be inconsistent during the initial track load and layout

If possible, find and fix that issue

@hkmoon

This comment has been minimized.

Copy link
Contributor

hkmoon commented Aug 23, 2018

I thought that initial zoom level prevents from computing initial actual height if the initial height does not meet a certain criteria or simply the default zoom ratio might override the initial height. Please, have a look the zoom level logic flow. Removing the default ratio helped this situation in my case.

In the case of having wrong height, when I zoom in or out by click +/- icons. It computes the right height after just one clicking.

I hope this explanation might help you find the problem.

@hkmoon

This comment has been minimized.

Copy link
Contributor

hkmoon commented Aug 23, 2018

Please unzip the attached file in the JBrowse root and test with the below url:

http://localhost/jbrowse/?data=data&loc=dd_Smes_g4_131%3A103817..105394&tracks=dd_Smes_v1%2Cdd_Smed_v6&highlight=&tracklist=1&highres=2&nav=1&overview=1

data.zip

Thank you for making and keeping track of this issue!

@garrettjstevens

This comment has been minimized.

Copy link
Contributor

garrettjstevens commented Aug 31, 2018

I think I see the problem here, but I'm not sure the best way to go about solving it. JBrowse thinks the features are wider than they really are, so it's initially leaving room for them to stack even though they don't need it. In the code below, it grabs the feature rectangle, and scales it to create the actual rectangle. But in the _getFeatureRectangle code (in src/JBrowse/View/FeatureGlyph/Box.js) it increases the with of the rectangle if the label is wider than the rectangle. Then, when it gets re-scaled below, there is lots of extra room for the labels even though it's not needed.

layoutFeature: function( viewArgs, layout, feature ) {
var fRect = this._getFeatureRectangle( viewArgs, feature );
var scale = viewArgs.scale;
var leftBase = viewArgs.leftBase;
var startbp = fRect.l/scale + leftBase;
var endbp = (fRect.l+fRect.w)/scale + leftBase;
fRect.t = layout.addRect(
feature.id(),
startbp,
endbp,
fRect.h,
feature
);

So for example _getFeatureRectangle originally creates something like this:
orig

Which then gets scaled horizontally, so JBrowse thinks it looks like this:
stretch

But the labels don't actually get bigger, so it really looks like this:
stretchreal

All that extra space that JBrowse thinks it needs is what's causing the problem. That's why changing the scale fixed it originally, since it didn't have to do any of those scale conversions. Also, if you change the track modes to "collapsed" so there are not labels the problem goes away as well.

It seems odd that _getFeatureRectangle scales the features down (via bpToX) and then the features get scaled back up in layoutFeature, but there might be a reason for that I'm not seeing.

@hkmoon

This comment has been minimized.

Copy link
Contributor

hkmoon commented Aug 31, 2018

@garrettjstevens Thank you so much for digging this out and explaining the in-depth code analysis. If you think it's not a problem for most of other cases, please, close this issue. However, I have to use the scale removed version for our web service until this problem is fixed since our web page embeds JBrowse in <IFrame> where the size is limited.

@hkmoon

This comment has been minimized.

Copy link
Contributor

hkmoon commented Sep 14, 2018

As initView has fixed scale factor, it makes inconsistent height if the queried startbp and endbp are not matched to this factor. Therefore, sometimes JBrowse assumes that the view contains many features in the range. N * features in the range produces big margin.

I would propose to compute zoomLevel factor with below codes if startbp and endbp are given. Otherwise, we use refseq length directly.

One other option would be make the track rendering process delayed until getting the proper zoom level factor is computed.

How would you guys think it is possible?

this.pxPerBp = Math.min(this.getWidth() / (endbp - startbp), this.maxPxPerBp );
this.curZoom = Util.findNearest(this.zoomLevels, this.pxPerBp);

pxPerBp seems to be very important factor which will be used all the features. However, I am not sure if getWidth() gives actual window width or not, before UI initialization. It might be quite tricky to solve it.

sizeInit: function() {
this.overviewBox = dojo.marginBox(this.overview);
this.elemBox = { h: this.elem.offsetHeight, w: this.elem.offsetWidth };
//scale values, in pixels per bp, for all zoom levels
var desiredZoomLevels = [1/500000, 1/200000, 1/100000, 1/50000, 1/20000, 1/10000, 1/5000, 1/2000, 1/1000, 1/500, 1/200, 1/100, 1/50, 1/20, 1/10, 1/5, 1/2, 1, 2, 5, 10, 20 ];
this.zoomLevels = [];
for( var i = 0; i < desiredZoomLevels.length; i++ ) {
var zlevel = desiredZoomLevels[i];
if( zlevel < this.maxPxPerBp )
this.zoomLevels.push( zlevel );
else
break; // once get to zoom level >= maxPxPerBp, quit
}
this.zoomLevels.push( this.maxPxPerBp );
//make sure we don't zoom out too far
while (((this.ref.end - this.ref.start) * this.zoomLevels[0])
< this.getWidth()) {
this.zoomLevels.shift();
}
this.zoomLevels.unshift(this.getWidth() / (this.ref.end - this.ref.start));
//width, in pixels, of stripes at min zoom (so the view covers
//the whole ref seq)
this.minZoomStripe = this.regularStripe * (this.zoomLevels[0] / this.zoomLevels[1]);
this.curZoom = 0;
while (this.pxPerBp > this.zoomLevels[this.curZoom])
this.curZoom++;
this.maxLeft = this.bpToPx(this.ref.end+1) - this.getWidth();
delete this.stripePercent;
//25, 50, 100 don't work as well due to the way scrollUpdate works
var possiblePercents = [20, 10, 5, 4, 2, 1];
for (var i = 0; i < possiblePercents.length; i++) {
// we'll have (100 / possiblePercents[i]) stripes.
// multiplying that number of stripes by the minimum stripe width
// gives us the total width of the "container" div.
// (or what that width would be if we used possiblePercents[i]
// as our stripePercent)
// That width should be wide enough to make sure that the user can
// scroll at least one page-width in either direction without making
// the container div bump into the edge of its parent element, taking
// into account the fact that the container won't always be perfectly
// centered (it may be as much as 1/2 stripe width off center)
// So, (this.getWidth() * 3) gives one screen-width on either side,
// and we add a regularStripe width to handle the slightly off-center
// cases.
// The minimum stripe width is going to be halfway between
// "canonical" zoom levels; the widest distance between those
// zoom levels is 2.5-fold, so halfway between them is 0.7 times
// the stripe width at the higher zoom level
if (((100 / possiblePercents[i]) * (this.regularStripe * 0.7))
> ((this.getWidth() * 3) + this.regularStripe)) {
this.stripePercent = possiblePercents[i];
break;
}
}
if ( ! this.stripePercent ) {
console.warn("stripeWidth too small: " + this.stripeWidth + ", " + this.getWidth());
this.stripePercent = 1;
}
var oldX;
var oldStripeCount = this.stripeCount;
if (oldStripeCount) oldX = this.getX();
this.stripeCount = Math.round(100 / this.stripePercent);
this.scrollContainer.style.width =
(this.stripeCount * this.stripeWidth) + "px";
this.zoomContainer.style.width =
(this.stripeCount * this.stripeWidth) + "px";
var blockDelta;
if (oldStripeCount && (oldStripeCount != this.stripeCount)) {
blockDelta = Math.floor((oldStripeCount - this.stripeCount) / 2);
var delta = (blockDelta * this.stripeWidth);
var newX = this.getX() - delta;
this.offset += delta;
this.updateStaticElements( { x: newX } );
this.rawSetX(newX);
}
// update the sizes for each of the tracks
this.trackIterate(function(track, view) {
track.sizeInit(view.stripeCount,
view.stripePercent,
blockDelta);
});
var newHeight =
this.trackHeights && this.trackHeights.length
? Math.max(
dojof.reduce( this.trackHeights, '+') + this.config.trackPadding * this.trackHeights.length,
this.getHeight()
)
: this.getHeight();
this.scrollContainer.style.height = newHeight + "px";
this.containerHeight = newHeight;
var refLength = this.ref.end - this.ref.start;
if( refLength < 0 )
throw new Error("reference sequence "+this.ref.name+" has an invalid start coordinate, it is greater than its end coordinate.");
var posSize = document.createElement("div");
posSize.className = "overview-pos";
posSize.appendChild(document.createTextNode(Util.addCommas(this.ref.end)));
posSize.style.visibility = "hidden";
this.overview.appendChild(posSize);
// we want the stripes to be at least as wide as the position labels,
// plus an arbitrary 20% padding so it's clear which grid line
// a position label corresponds to.
var minStripe = posSize.clientWidth * 1.2;
this.overviewPosHeight = posSize.clientHeight * 1.2;
this.overview.removeChild(posSize);
for (var n = 1; n < 30; n++) {
//http://research.att.com/~njas/sequences/A051109
// JBrowse uses this sequence (1, 2, 5, 10, 20, 50, 100, 200, 500...)
// as its set of zoom levels. That gives nice round numbers for
// bases per block, and it gives zoom transitions that feel about the
// right size to me. -MS
this.overviewStripeBases = (Math.pow(n % 3, 2) + 1) * Math.pow(10, Math.floor(n/3));
this.overviewStripes = Math.ceil(refLength / this.overviewStripeBases);
if ((this.overviewBox.w / this.overviewStripes) > minStripe) break;
if (this.overviewStripes < 2) break;
}
// update our overview tracks
var overviewStripePct = 100 / (refLength / this.overviewStripeBases);
var overviewHeight = 0;
this.overviewTrackIterate(function (track, view) {
track.clear();
track.sizeInit(view.overviewStripes,
overviewStripePct);
track.showRange(0, view.overviewStripes - 1,
view.ref.start-1, view.overviewStripeBases,
view.overviewBox.w /
(view.ref.end - view.ref.start));
});
this.updateOverviewHeight();
this.updateScroll();
},

@hkmoon

This comment has been minimized.

Copy link
Contributor

hkmoon commented Sep 17, 2018

Hi,

The issue occurs when initSize() -> setLocation(start, end) -> the zoomLevel keeps the initial value because the features are not loaded completely since the track loading is deferred.

this.centerAtBase((startbp + endbp) / 2, true);

If the below code is added after the above line,

    thisB = this;
    if( ! this._centerAtBaseTimeout )
        this._centerAtBaseTimeout = window.setTimeout(
            function() {
                thisB.centerAtBase((startbp + endbp) / 2, true);
                delete thisB._centerAtBaseTimeout;
            }, 2000 );

then, it shows the proper height.

Probably, if there is an event handler to check the all the tracks are loaded completely, we can put the above code there instead of using timeout(). Could you let me know which event handler catches an event when all the tracks are loaded completely if there is?

@cmdcolin

This comment has been minimized.

Copy link
Contributor

cmdcolin commented Nov 9, 2018

This is aiming to get implemented as part of paired read views since it became relevant there

@cmdcolin cmdcolin added the has pullreq label Nov 9, 2018

@cmdcolin cmdcolin added this to the 1.16.0 milestone Nov 9, 2018

@cmdcolin

This comment has been minimized.

Copy link
Contributor

cmdcolin commented Dec 7, 2018

The PR that was suggested in #1187 was incorporated but I haven't tested this specific aspect of it, if anyone wants to try please let me know

@cmdcolin

This comment has been minimized.

Copy link
Contributor

cmdcolin commented Dec 8, 2018

Looks like this aspect was fixed according to the data directory. Great work all

@cmdcolin cmdcolin closed this Dec 8, 2018

@hkmoon

This comment has been minimized.

Copy link
Contributor

hkmoon commented Dec 10, 2018

Sorry that it's off-the-topic. ;)

JBrowse is cited in our recent publication (https://www.ncbi.nlm.nih.gov/pubmed/30496475). Thank you so much for your great work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment