Skip to content
This repository has been archived by the owner on Jun 26, 2022. It is now read-only.

Add minimum rectangle width #6

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

Conversation

sftse
Copy link

@sftse sftse commented Oct 25, 2021

If the canvas is not sufficiently proportioned, the svg value written as width can become negative and the rectangles disappear.
This is initially confusing. Adding a catch for such cases ensures that a best effort is made to still render something.
Arguably this check could be done in plotters.

@facorread
Copy link
Member

facorread commented Jun 17, 2022

I would suggest closing #6 in favor of #7.

Thanks for contributing.

@shinmili
Copy link

shinmili commented Jun 17, 2022

No, the issue for #7 is not because of negative width/height, so I think this is another one. Maybe this is a fix for plotters-rs/plotters#300?

@facorread
Copy link
Member

🤔 You're right. This means that what's going to happen is a combination of both PRs.

Do you think you would like to contribute an updated version of #7, together with an expanded test?

@shinmili
Copy link

I tried the code in plotters-rs/plotters#300 with this patch applied. It generated a histogram of 1-px-wide bins (below), but that's still different from this png provided by the original issue author.

histogram

I think this patch is to be considered as a workaround (if this is really for the issue!) and I'm not willing to continue this. Sorry 🙇 The true cause of negative width/height must be searched from the caller side, plotters-rs/plotters.

@AaronErhardt
Copy link
Member

I agree, a minimum width shouldn't be implemented by each backend individually but by plotters itself. I appreciate the effort but I'm in favor of closing this PR. (But it seems like I don't have the permission to close it, I don't have write access here).

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

Successfully merging this pull request may close these issues.

4 participants