-
Notifications
You must be signed in to change notification settings - Fork 48
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
[WIP] Pie chart #204
base: main
Are you sure you want to change the base?
[WIP] Pie chart #204
Conversation
@tusharpm Looks good, yeah, right now there is no way to disable rendering of ticks and grid lines. But I can add those, shouldn't be hard - just want to make sure it is done the right way. |
I will comment on changes once I look at the shader etc. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code's good. I will try to make a PR that lets user or at least internal API to turn off grid and traditional 2d plot things that aren't helpful with Pies.
Have you thought about how to put some text related to pie %'s ?
For the pie %, I'm not sure about a GLSL way of rendering text. (I'll try looking that up). |
perhaps something like |
I have created the #205 with necessary changes You can apply the following diff on your PR to rendering pie without axes. diff --git a/src/api/c/chart.cpp b/src/api/c/chart.cpp
index e63af51..d21ca75 100644
--- a/src/api/c/chart.cpp
+++ b/src/api/c/chart.cpp
@@ -180,6 +180,7 @@ fg_err fg_append_pie_to_chart(fg_chart pChart, fg_pie pPie)
ARG_ASSERT(1, (pPie!=0));
getChart(pChart)->addRenderable(getPie(pPie)->impl());
+ getChart(pChart)->setAxesVisibility(false);
}
CATCHALL
@@ -279,6 +280,7 @@ fg_err fg_add_pie_to_chart(fg_pie* pPie, fg_chart pChart,
common::Pie* pie = new common::Pie(pNSectors, (forge::dtype)pType);
chrt->addRenderable(pie->impl());
+ chrt->setAxesVisibility(false);
*pPie = getHandle(pie);
}
CATCHALL |
@tusharpm Great, I see that you have disabled axes rendering now. Sorry, I didn't get notification for that push, so I was under the impression you are still working on it. What are your thoughts on legends ? |
I had actually missed the notification for the PR you merged earlier. I'm sorry for the confusion. I haven't looked into legends, yet. |
@tusharpm Sorry about the delay, I will think about possible ideas, but as of now, I can only think of it to get the lists of texts(legends) and percentages from user. The legends from plotting are slightly different because each legend is set individually by each item of 2d chat. However, in the case of pie, we need to push all these into a single item, thus would involve a vector of strings or something like that. Percentages will be a tricky one for sure. Let me take a look this PR changes once again. You can also ping me on slack - https://arrayfire-org.slack.com/messages/C6QTLM3M0 for a real time discussion. Thank you. |
I have rebased it from latest master and added remove API for pie renderables too. |
I will work on this some time in 2024 to revive this feature. So will make this into draft up until the point. |
What kind of change does this PR introduce?
Feature.
What is the current behavior?
#68 lists pie chart as an option for statistical plots.
What is the new behavior?
Add Pie chart.
Does this PR ensure that key events handling across different window toolkits is identical if it
has changes related to window toolkits?
N/A.
Does this PR introduce a breaking change?
No. Pie API is purely additive.
Please check if the PR fulfills these requirements
builds don't check for this yet).
Other information:
WIP:
The branch includes a CPU example - need to write the same for CUDA/OpenCL.
Need to find a formal way to disable grid and ticks for the chart object.
The API has the doc comments (similar to histogram).
Need to update README.md to reflect Pie chart support.