Profile scrollers

[emalysz]

No description provided.

[squarewave]

Okay, I'm going to start off with some broad comments here. I may dive into the details, but I think some conceptual reorganizing is a first priority.

First off, we should avoid creating a canvas that gets overlaid on another canvas. I'm not sure what optimizations we've made to limit what we actually have to draw with a big canvas, but my understanding is we basically have to process every pixel inside a canvas, at least when it comes to the compositor. It's not the end of the world, but if it's easy to avoid, we should. (And it is easy to avoid here.) So let's just use the main renderer, and add a rect for each overlay region that's partially filled:

let {
  tracks,
} = gState;
let {
  constrainedTimeBegin,
} = gOuterState;
if (isTopSlider) {
  renderer.pushRect(TIMELINE_UNSELECTED_GREY,
                    /*x:*/ 0, /*y:*/ 0,
                    /*width:*/ tracks.length, /*height:*/ absTimeToRelTime(constrainedTimeBegin),
                    TIMELINE_SELECTION_OVERLAY_DEPTH,
                    TIMELINE_SELECTION_OVERLAY_FILL);
} else {
  ...
}

There's a few things introduced in the snippet above though that I should call out. The first is that I made a gOuterState global for holding the state of our scrollers. This will make it easier to track and even reset them if we'd like to, and makes it clearer when reading the code that these are not local variables. I didn't wrap the state into the gState global, because in a sense this supersedes that. gState should only contain information from the range that hasn't been filtered out by the data in gOuterState.

The second thing is I invoke a function here which doesn't exist: absTimeToRelTime. Currently I do all conversion between coordinate systems inline, and it's a bit hard to follow. So we should make these more explicit. There are three Y coordinate systems that we have to deal with: absolute time (seconds since epoch), relative time (seconds since first entry in filtered data), and screen space (pixels from the top of the page). In general, I like to stay in time coordinates, and relative time coordinates where possible. This is because it's the functional space we're working in, rather than just the presentational space. In our case, we will want to store our filter range in absolute time. We can't do relative time, because in the case where we've filtered our entries down to a particular range, relative time is actually determined by the start of that range. I will just include all the coordinate conversion functions below. We should replace all of our coordinate conversion code with these functions. (I can do this as a followup, if you like.)

function absTimeToRelTime(absTime) {
  let { minTime } = gState;
  return absTime - minTime;
}

function relTimeToAbsTime(relTime) {
  let { minTime } = gState;
  return relTime + minTime;
}

function relTimeToScreenSpace(relTime) {
  let { rendererScale, rendererTranslate } = gState;
  return (relTime + rendererTranslate) * rendererScale;
}

function absTimeToScreenSpace(absTime) {
  return relTimeToScreenSpace(absTimeToRelTime(absTime));
}

function screenSpaceToRelTime(screenY) {
  let { rendererScale, rendererTranslate } = gState;
  return screenY / rendererScale - rendererTranslate;
}

function screenSpaceToAbsTime(screenY) {
  return relTimeToAbsTime(screenSpaceToRelTime(screenY));
}

So, in order to compute our constrainedTimeBegin value from a mouse coordinate, we would just do gOuterState.constrainedTimeBegin = screenSpaceToAbsTime(mouseY);. In order to filter our data to only things within the constrained time, we could do something like let constrainedData = gOuterState.unconstrainedData.filter(row => row.start > gOuterState.constrainedTimeBegin && row.start + row.duration < gOuterState.constrainedTimeEnd);. Does that all seem reasonable, and fit with what you've encountered so far in developing this?

[squarewave]

I haven't gone through everything yet, and I intend to go through more on (hopefully) Monday, but I will leave these comments here since I think there's a good chunk of things to be done addressing them.

One of the comments alluded to a bug I noticed, which I'll give STR for here:

1. Open a procmon log in the viewer
2. Refocus so the top is around, say, 5 seconds, and the bottom is around 10 seconds.
3. Keep refocusing the top to somewhere between 5 and 6 seconds
4. Notice that eventually, the top somehow actually refocuses past 6 seconds

[squarewave]

Let's move this outside of drawData calls. I think it would be preferable if this function does not modify gOuterState, and as much as possible restricts itself to thinking about only gState.

[squarewave]

Relative time, as the code has been informally treating it prior to this, was always relative to the minTime as defined by gState. If we use gOuterState here, then we're going to run into problems, and some of the bugs I'm running into feel like that's what's going on?

[squarewave]

Here is an example of the problem with absTimeToRelTime - we're subtracting constrainedRelative below to try to correct for the discrepancy, but we shouldn't need to do that. The renderer should be entirely in relative time, relative to gState.minTime, not gOuterState.minTime, because it shouldn't need to be aware of gOuterState. There are some leaky parts, like wanting to make sure that the timeline indicators and things like that use time relative to gOuterState, but I think that means we need another coordinate space, maybe "outer relative time" (absTimeToOuterRelTime). Is that making sense? (I could be wrong about this, but I think in the vast majority of cases things are simpler if we have relative time be relative to gState.minTime.)