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

fixes padding issue of box algorithm #1087

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

stu241636
Copy link

fixes ticket #866 by taking padding into account when calculating total area.

@stu241636 stu241636 force-pushed the box branch 3 times, most recently from 2c57e12 to f312f74 Compare November 8, 2024 14:54
@soerendomroes soerendomroes added the alg Affects a non-specific part of ELK's algorithms. label Nov 8, 2024
@soerendomroes soerendomroes added this to the Release 0.9.2 milestone Nov 8, 2024
@soerendomroes soerendomroes self-requested a review November 8, 2024 14:57
More changes to box algorithm
// add top and bottom padding to total area
totalArea += Math.sqrt(totalArea) * (padding.getBottom() + padding.getTop());
// add left and right padding
totalArea += Math.sqrt(totalArea) * (padding.getRight());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

left padding is missing here. Does adding the padding here help with the underlying issue? I.e. is this something we want finished and merged or does it not really help?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Taking the padding into account does not fully solve the issue since the desired aspect ratio is a ratio, which cannot be influenced concrete values (the padding). Hence, it helps make it a little better but does not fully solve the problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
alg Affects a non-specific part of ELK's algorithms.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants