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

center offset #37

Closed
llchan opened this issue Apr 14, 2017 · 8 comments
Closed

center offset #37

llchan opened this issue Apr 14, 2017 · 8 comments
Assignees

Comments

@llchan
Copy link

llchan commented Apr 14, 2017

I think the center func offset in axis.js is slightly off.

function center(scale) {
  var offset = scale.bandwidth() / 2;
  if (scale.round()) offset = Math.round(offset);
  return function(d) {
    return scale(d) + offset;
  };
}

Suppose bandwidth is 3. Then we expect the offset to be 1, to place it on the middle pixel. With the current implementation, you would get 1.5 or 2, depending on the rounding flag. I think the correct offset should be:

var offset = (scale.bandwidth() - 1) / 2;
@mbostock
Copy link
Member

Have you tested your propose change? Note that lines are rendered offset by half a pixel to produce crisp edges:

line = line.merge(tickEnter.append("line")
    .attr("stroke", "#000")
    .attr(x + "2", k * tickSizeInner)
    .attr(y + "1", 0.5)
    .attr(y + "2", 0.5));

@llchan
Copy link
Author

llchan commented Apr 14, 2017

I'm admittedly somewhat new to d3.js and I am not familiar with the build process, so i haven't tested the fix.

However, I made an example here to demonstrate what I'm seeing.
https://jsfiddle.net/vpb7aLpc/3/
Notice that the tick marks are slightly off-center. The console output shows e.g. translate by 8 pixels for a 15-pixel-wide bar, which I think is off by 1. The half pixel shift mentioned above only puts it squarely in that pixel.

Am I misunderstanding something?

@mbostock
Copy link
Member

mbostock commented Apr 15, 2017

Here’s a pixel grid overlaid on your example:

untitled 2

And without rangeRound for comparison:

untitled 1

I see what you mean now. If the bands are 15px wide, then there should be 7px on either side of the tick (7 + 1 + 7 = 15). Instead there is 8px on the left side and 6px on the right side. And both range and rangeRound should produce the same result.

@mbostock
Copy link
Member

mbostock commented Apr 16, 2017

Hmm. Making the proposed change fixes the above example, but negatively affects point scales without rangeRound. Here’s before (with the ticks in red, so they can be seen overlaid on top of the domain path):

untitled 4

And after:

untitled 3

However, we can fix this fairly easily like so:

var offset = Math.max(0, scale.bandwidth() - 1) / 2;

@mbostock
Copy link
Member

Okay, fix pending in #38. Look good?

@mbostock mbostock self-assigned this Apr 16, 2017
@llchan
Copy link
Author

llchan commented Apr 17, 2017

Appreciate the time taken to look into this! A few comments:

  • The max with 0 would only come into play if bandwidth is < 1 pixel, in which case the shift-by-half-pixel logic is going to make it look funny anyways. So I think it might be unnecessary?
  • I am a little confused about the second set of screenshots: how come the 0/2 tick labels are now shifted to the edges of the band? Is this just a screenshot issue?

@mbostock
Copy link
Member

The bandwidth is zero for point scales (d3.scalePoint), which is what I was discussing in #37 (comment).

@llchan
Copy link
Author

llchan commented Apr 18, 2017

Got it. I wasn't familiar with that scale so I'd glossed over the phrase "point scale" when I read your comment. I think it looks good to me.

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

No branches or pull requests

2 participants