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

Fix rendering of negative values in stacked bar diagram #37924

Merged
merged 3 commits into from Jul 30, 2020
Merged

Fix rendering of negative values in stacked bar diagram #37924

merged 3 commits into from Jul 30, 2020

Conversation

dminor
Copy link
Contributor

@dminor dminor commented Jul 22, 2020

Currently, negative values are treated the same way as positive values when
rendering stacked bar diagrams, which can lead to rendering issues, because
we can end up with negative lengths and incorrect scaling.

This changes the rendering to track the negative values separately and render
them in a group below the x axis. The rendering for positive values is
unchanged, above the x axis.

Description

Rendering of negative values without this patch:
qgis-stackedbarnegative-unpatched

Rendering of negative values with this patch:
qgis-stackedbarnegative-patched

@github-actions github-actions bot added this to the 3.16.0 milestone Jul 22, 2020
@nyalldawson
Copy link
Collaborator

@dminor code looks good to me, thanks!

Just looking at the rendered reference images, it looks like there's an issue with the axis height when one or more values is negative. We should ignore negative values when calculating the axis height to avoid this. But I'm also wondering if we should extend the vertical axis down to fit negative values too?

One last thing -- can you turn on the "show label candidates" option in the project label engine settings, and confirm that the label candidate boxes correctly contain the negative components of the bar? (If they don't then the label/diagram overlap conflict code won't be applied correctly)

Currently, negative values are treated the same way as positive values when
rendering stacked bar diagrams, which can lead to rendering issues, because
we can end up with negative lengths and incorrect scaling.

This changes the rendering to track the negative values separately and render
them in a group below the x axis. The rendering for positive values is
unchanged, above the x axis.
@dminor
Copy link
Contributor Author

dminor commented Jul 29, 2020

@nyalldawson thank you for having a look!

The label placement does look problematic when I tested using longer labels, so I'll have to shift everything up rather than playing with the currentOffset. Thank you for catching that. I'm not able to find the "show label candidates" option you mentioned, could you please give me more detailed instructions? I'd like to test with that to make sure I have things fixed.

I think it makes sense to extend the axis down next to the negative values. If all of the values are negative, we would just have the horizontal portion of the axis bar rendered, which is kind of strange looking.

@nyalldawson
Copy link
Collaborator

@dminor
Here's how you get to the setting:

Peek 2020-07-29 11-57

@dminor
Copy link
Contributor Author

dminor commented Jul 29, 2020

Thank you!

@dminor
Copy link
Contributor Author

dminor commented Jul 30, 2020

Here is an image with the updated rendering and label candidates enabled.

This doesn't include extending the axis along the negative values. That isn't done for histograms either, and I think we should them the same way. I can look at changing both in a future pull request if you think that makes sense.

qgis-stackedbarnegative-updated

@nyalldawson
Copy link
Collaborator

Looks good to me! Is it ready for a re-review?

@dminor
Copy link
Contributor Author

dminor commented Jul 30, 2020

Looks good to me! Is it ready for a re-review?

Yes please!

@nyalldawson nyalldawson merged commit 1ed8e41 into qgis:master Jul 30, 2020
@nyalldawson
Copy link
Collaborator

Thanks @dminor !

@dminor
Copy link
Contributor Author

dminor commented Jul 30, 2020

Thanks for the review!

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

Successfully merging this pull request may close these issues.

None yet

2 participants