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

enable 3d pie #1282

Closed
wants to merge 1 commit into from
Closed

enable 3d pie #1282

wants to merge 1 commit into from

Conversation

ghost
Copy link

@ghost ghost commented Apr 5, 2014

by extend the pie plugin of the libary and add depth paramater to the
plugin

by extend the pie plugin of the libary and add  depth paramater to the
plugin
@dnschnur dnschnur added the triage label Apr 6, 2014
@jrouillard
Copy link

I think it is the same problem than #1065
I like your solution better because it is much lighter, but it has more problems too : stacking 2 pie charts doesnt make a real 3d pie, but for a little pie it give the impression of 3d.

edit:
I talked too fast, it draws multiple pies to render the 3d, not just 2 and this is a clever way to do it. I definitely like it better. However, it isnt compatible with the donut, the way the donut is currently drawn (by suppressing the inner circle, check #1225), and it seems like the highlights could be better handled, by highlighting the whole slice and not just the top.

The stroke doesnt seem to appear, too:

if (options.series.pie.stroke.width > 0 && 1==2)

It seems like you dont want a stroke to appear.

jsfiddle to check the result:
http://jsfiddle.net/b5Znq/

@ghost
Copy link
Author

ghost commented Apr 7, 2014

first of all you thank u for the quick reply
i will try to answer by points :
1.I can't remember why i'd remove the stroke back then [silly thing ]-i will return the stroke and check the result.
2.Highlighting is also true i didn't manage to handle so well i guess [again i should try & solve it]
3.about the Donut i think we can only draw the outside of the pie & then it might solve it [what do u think?]

@jrouillard
Copy link

Personnally I think it is too much of a change, it should be handled by the flot maintainer (dns in this case). There are so many ways to implement that, and that issue seems to have a milestone anyway (0.9, later this year maybe). Nothing prevent you from implementing this on your own: at this point, I think every flot enthusiast did a 3d pie. For your higlighting issue, you should check the drawHighlight() method, but as I said there are so many ways to implement something. Here is my 'highlighting' :
test

ctx.restore();

// draw slice outlines

if (options.series.pie.stroke.width > 0) {
if (options.series.pie.stroke.width > 0 && 1==2) {
Copy link
Member

Choose a reason for hiding this comment

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

This would always return false ...

@nschonni
Copy link
Member

nschonni commented Apr 8, 2014

Closing since this wouldn't go into the master 0.8.x branch. I'll leave it up to say @dnschnur if he's interested in this for the 0.9-work branch.

@nschonni nschonni closed this Apr 8, 2014
@dnschnur
Copy link
Member

I'm fine with this change; 3D is a natural thing for the pie plugin to have. But, as Nick said, it belongs on the 0.9-work branch, and the implementation needs to work with everything that the pie plugin currently supports; donut holes, selection, etc. If that requires other changes then they should be broken into their own commits within the pull request.

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

Successfully merging this pull request may close these issues.

None yet

4 participants