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

Uneven pixel sizes / spacing with pointPadding #5755

Closed
KacperMadej opened this Issue Sep 30, 2016 · 15 comments

Comments

Projects
None yet
4 participants
@KacperMadej
Contributor

KacperMadej commented Sep 30, 2016

Expected behaviour

Constant spacing between bars across all categories.

Actual behaviour

Sometimes it's 1 pixel, sometimes it's 0 pixels.

Live demo with steps to reproduce

http://jsfiddle.net/vjbjpka6/1/

Affected browser(s)

Best to view in Chrome, but the issue is a general one.

@TorsteinHonsi

This comment has been minimized.

Show comment
Hide comment
@TorsteinHonsi

TorsteinHonsi Oct 20, 2016

Collaborator

It is important to set pointPadding to 0 if you want to achieve tightly packed columns. If it is 0, columns will consider the next column when doing antialiasing. Otherwise, it will just round off to the nearest pixel: http://jsfiddle.net/highcharts/vjbjpka6/5/

Collaborator

TorsteinHonsi commented Oct 20, 2016

It is important to set pointPadding to 0 if you want to achieve tightly packed columns. If it is 0, columns will consider the next column when doing antialiasing. Otherwise, it will just round off to the nearest pixel: http://jsfiddle.net/highcharts/vjbjpka6/5/

@MiroLiska

This comment has been minimized.

Show comment
Hide comment
@MiroLiska

MiroLiska Oct 24, 2016

Hi Torstein. The problem is that I do not want to achieve tightly packed columns. I "simply" want that the columns (or bars) have always the same spacing between them. If some of the spaces are 0, and some 1 pixel wide like now, the chart looks buggy. The current rounding logic should be replaced by something more sophisticated. Please do not forget that the chart may be placed in a resizable container. I have no idea whether the spacing should be set to 0 or 5 pixels. The same way I do not care how wide are the columns (or bars) themselves, your algorithm should be calculating all of these things.

MiroLiska commented Oct 24, 2016

Hi Torstein. The problem is that I do not want to achieve tightly packed columns. I "simply" want that the columns (or bars) have always the same spacing between them. If some of the spaces are 0, and some 1 pixel wide like now, the chart looks buggy. The current rounding logic should be replaced by something more sophisticated. Please do not forget that the chart may be placed in a resizable container. I have no idea whether the spacing should be set to 0 or 5 pixels. The same way I do not care how wide are the columns (or bars) themselves, your algorithm should be calculating all of these things.

@TorsteinHonsi

This comment has been minimized.

Show comment
Hide comment
@TorsteinHonsi

TorsteinHonsi Oct 25, 2016

Collaborator

The current rounding logic was designed to make sure that all columns have the same width or spacing, but it looks like this too is broken now: http://jsfiddle.net/highcharts/8v85hbcg/

The white stripes appear when the right side of a column rounds down, and the left side of the next column rounds up.

Collaborator

TorsteinHonsi commented Oct 25, 2016

The current rounding logic was designed to make sure that all columns have the same width or spacing, but it looks like this too is broken now: http://jsfiddle.net/highcharts/8v85hbcg/

The white stripes appear when the right side of a column rounds down, and the left side of the next column rounds up.

@TorsteinHonsi TorsteinHonsi reopened this Oct 25, 2016

@TorsteinHonsi

This comment has been minimized.

Show comment
Hide comment
@TorsteinHonsi

TorsteinHonsi Oct 25, 2016

Collaborator

Consider the following example: http://jsfiddle.net/highcharts/8v85hbcg/1/

In this case, the data is laid out so that each column is ~1px wide, and the gap between them is also ~1px. If we are to normalize this, we need to put in an extra pixel from time to time. Currently this is done on the columns, and the result is unfortunately quite visible. Alternatively we could add it to the gaps, but it would be equally visible.

Collaborator

TorsteinHonsi commented Oct 25, 2016

Consider the following example: http://jsfiddle.net/highcharts/8v85hbcg/1/

In this case, the data is laid out so that each column is ~1px wide, and the gap between them is also ~1px. If we are to normalize this, we need to put in an extra pixel from time to time. Currently this is done on the columns, and the result is unfortunately quite visible. Alternatively we could add it to the gaps, but it would be equally visible.

@MiroLiska

This comment has been minimized.

Show comment
Hide comment
@MiroLiska

MiroLiska Oct 25, 2016

Hi. My problem is actually the gap between individual series which is due to rounding "randomly looking".
See the rounding effect in the current solution at http://jsfiddle.net/af1hvda0/ and my solution at http://jsfiddle.net/af1hvda0/1/ .

MiroLiska commented Oct 25, 2016

Hi. My problem is actually the gap between individual series which is due to rounding "randomly looking".
See the rounding effect in the current solution at http://jsfiddle.net/af1hvda0/ and my solution at http://jsfiddle.net/af1hvda0/1/ .

@TorsteinHonsi

This comment has been minimized.

Show comment
Hide comment
@TorsteinHonsi

TorsteinHonsi Oct 25, 2016

Collaborator
Collaborator

TorsteinHonsi commented Oct 25, 2016

@MiroLiska

This comment has been minimized.

Show comment
Hide comment
@MiroLiska

MiroLiska Oct 26, 2016

Hi. I have programmed my plugin the way that all the columns are equal and all the gaps as well. Plugin will not react to columns narrower than 1 px, e.g. the examples with 500 columns displayed on the 300px container are handled as it was before. Please see http://jsfiddle.net/af1hvda0/2/ and try to create a normal use case where the plugin delivers worse results than the current solution.

MiroLiska commented Oct 26, 2016

Hi. I have programmed my plugin the way that all the columns are equal and all the gaps as well. Plugin will not react to columns narrower than 1 px, e.g. the examples with 500 columns displayed on the 300px container are handled as it was before. Please see http://jsfiddle.net/af1hvda0/2/ and try to create a normal use case where the plugin delivers worse results than the current solution.

@TorsteinHonsi

This comment has been minimized.

Show comment
Hide comment
@TorsteinHonsi

TorsteinHonsi Oct 26, 2016

Collaborator

Thanks for sharing! You're right, it probably won't give worse results, only fix some cases. Which is of course an improvement.

But in a case like this, http://jsfiddle.net/highcharts/af1hvda0/3/, the gaps are still uneven... Does your plugin only apply to multiple columns?

Collaborator

TorsteinHonsi commented Oct 26, 2016

Thanks for sharing! You're right, it probably won't give worse results, only fix some cases. Which is of course an improvement.

But in a case like this, http://jsfiddle.net/highcharts/af1hvda0/3/, the gaps are still uneven... Does your plugin only apply to multiple columns?

@MiroLiska

This comment has been minimized.

Show comment
Hide comment
@MiroLiska

MiroLiska Oct 26, 2016

Yes, my problem is that the gaps between series were "random" for quite standard looking charts.

if (nuDone <= 1 || ret.width < 1) {
  return ret; // there is nothing to optimize
}

MiroLiska commented Oct 26, 2016

Yes, my problem is that the gaps between series were "random" for quite standard looking charts.

if (nuDone <= 1 || ret.width < 1) {
  return ret; // there is nothing to optimize
}
@matixmatix

This comment has been minimized.

Show comment
Hide comment
@matixmatix

matixmatix Feb 24, 2017

I'm also having difficulties making my charts to have the same spacing (check the image below). Are there any near future plans to address this issue?
image

matixmatix commented Feb 24, 2017

I'm also having difficulties making my charts to have the same spacing (check the image below). Are there any near future plans to address this issue?
image

@TorsteinHonsi

This comment has been minimized.

Show comment
Hide comment
@TorsteinHonsi

TorsteinHonsi Feb 28, 2017

Collaborator

The logic in question is in the crispCol function. As you can see from the code, it rounds the edges to the nearest pixel in order to render crisp on screen.

This is how it looks by default: http://jsfiddle.net/uacuoybm/

And this is how it looks when overriding the crisping function: http://jsfiddle.net/uacuoybm/1/. As you see it is now equal spacing, but it looks more blurry.

Perhaps we should just add an option for this? In some cases it may look better with a little blur than inequal spaces? What do you think?

Collaborator

TorsteinHonsi commented Feb 28, 2017

The logic in question is in the crispCol function. As you can see from the code, it rounds the edges to the nearest pixel in order to render crisp on screen.

This is how it looks by default: http://jsfiddle.net/uacuoybm/

And this is how it looks when overriding the crisping function: http://jsfiddle.net/uacuoybm/1/. As you see it is now equal spacing, but it looks more blurry.

Perhaps we should just add an option for this? In some cases it may look better with a little blur than inequal spaces? What do you think?

@matixmatix

This comment has been minimized.

Show comment
Hide comment
@matixmatix

matixmatix Feb 28, 2017

@TorsteinHonsi I would be happy if I could opt into this behaviour! In my case, a little blur looks a lot better than uneven spacing.

matixmatix commented Feb 28, 2017

@TorsteinHonsi I would be happy if I could opt into this behaviour! In my case, a little blur looks a lot better than uneven spacing.

@MiroLiska

This comment has been minimized.

Show comment
Hide comment
@MiroLiska

MiroLiska Apr 28, 2017

Hello Torstein

I have seen that you have closed this issue, now having the option 'crisp' for column charts. However, this option does not solve my problem at all. It may be a solution for a problem reported by matixmatix, I am not sure, but surely it does not do anything with the chart I have prepared on http://jsfiddle.net/vjbjpka6/1/ - that one still looks ugly when using 'crisp' set to false. I beg you to have a look at the solution I have presented here at http://jsfiddle.net/af1hvda0/2/ -it may not be the best one but I have not seen anything better until now.

Thanks in advance

Miro

MiroLiska commented Apr 28, 2017

Hello Torstein

I have seen that you have closed this issue, now having the option 'crisp' for column charts. However, this option does not solve my problem at all. It may be a solution for a problem reported by matixmatix, I am not sure, but surely it does not do anything with the chart I have prepared on http://jsfiddle.net/vjbjpka6/1/ - that one still looks ugly when using 'crisp' set to false. I beg you to have a look at the solution I have presented here at http://jsfiddle.net/af1hvda0/2/ -it may not be the best one but I have not seen anything better until now.

Thanks in advance

Miro

@TorsteinHonsi

This comment has been minimized.

Show comment
Hide comment
@TorsteinHonsi

TorsteinHonsi May 2, 2017

Collaborator

Thanks for your suggestion Miro!

As I understand it your plugin aims for equal distance between the columns by analyzing neighbour columns? I think there are still some fail cases, like http://jsfiddle.net/highcharts/af1hvda0/5/ where it apparently doesn't analyze other columns in the same series.

It's a nice concept, and it should probably be doable to make this work all over. There's one logic thing to be decided though - at some point we need to choose between equal distances or equal column widths. But we can do it the simple way and leave that as an option...

Collaborator

TorsteinHonsi commented May 2, 2017

Thanks for your suggestion Miro!

As I understand it your plugin aims for equal distance between the columns by analyzing neighbour columns? I think there are still some fail cases, like http://jsfiddle.net/highcharts/af1hvda0/5/ where it apparently doesn't analyze other columns in the same series.

It's a nice concept, and it should probably be doable to make this work all over. There's one logic thing to be decided though - at some point we need to choose between equal distances or equal column widths. But we can do it the simple way and leave that as an option...

@MiroLiska

This comment has been minimized.

Show comment
Hide comment
@MiroLiska

MiroLiska May 3, 2017

Hi

This is not a fail case. I do not optimize this case at all, I leave your standard behavior in this case:

if (nuDone <= 1 || ret.width < 1) {
return ret; // there is nothing to optimize
}

My problem were random gaps between series with a few values and not for charts with one series with hundreds of values - this was probably the problem of the other guy, but not mine. Please have again a look at http://jsfiddle.net/af1hvda0/2/ - this is chart which is definitively not going to extremes - it has just 3 series x 7 values. And it looks buggy with standard highcharts solution - the gaps between series are random.

My solution solves this case and even some extreme ones like http://jsfiddle.net/af1hvda0/6/ , although this is definitively not my goal.

MiroLiska commented May 3, 2017

Hi

This is not a fail case. I do not optimize this case at all, I leave your standard behavior in this case:

if (nuDone <= 1 || ret.width < 1) {
return ret; // there is nothing to optimize
}

My problem were random gaps between series with a few values and not for charts with one series with hundreds of values - this was probably the problem of the other guy, but not mine. Please have again a look at http://jsfiddle.net/af1hvda0/2/ - this is chart which is definitively not going to extremes - it has just 3 series x 7 values. And it looks buggy with standard highcharts solution - the gaps between series are random.

My solution solves this case and even some extreme ones like http://jsfiddle.net/af1hvda0/6/ , although this is definitively not my goal.

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