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

Error on node with 2 children #15

Open
tgillet1 opened this Issue Apr 8, 2012 · 6 comments

Comments

Projects
None yet
8 participants
@tgillet1

tgillet1 commented Apr 8, 2012

Perhaps I misunderstood something in how to create an appropriate structure, but I found that when I had just 2 children for a node that an error occurred on line 523 of bubbletree.js. I found that on line 181 if node.right == node.left (when there are two nodes at the level), node.right is set to undefined. Then on line 523 node.right.amount is accessed but node.right is undefined, causing the error. I simply included a node.right==undefined check around the statement calculating rad2 and removed the node.right.amount component of the Math.max call in that case. My test case worked after making that change.

@PhilippFS

This comment has been minimized.

Show comment
Hide comment
@PhilippFS

PhilippFS Sep 1, 2012

For those of you without programming skills, here's the solution in code. Thanks a lot, tgillet1!

            if (node.right==undefined) {
                rad2 = 0 - Math.max(
                    //hw *0.8 - tgtScale * (a2rad(node.parent.amount)+a2rad(node.amount)), // maximum visible part
                    hw * 0.8 - tgtScale * (a2rad(node.parent.amount) + a2rad(Math.max(node.amount*1.15 + node.maxChildAmount*1.15, node.left.amount * 0.85))),
                    tgtScale*a2rad(node.parent.amount)*-1 + hw*0.15 // minimum visible part
                ) + hw;
            }
            else {
            rad2 = 0 - Math.max(
                    //hw *0.8 - tgtScale * (a2rad(node.parent.amount)+a2rad(node.amount)), // maximum visible part
                    hw * 0.8 - tgtScale * (a2rad(node.parent.amount) + a2rad(Math.max(node.amount*1.15 + node.maxChildAmount*1.15, node.left.amount * 0.85, node.right.amount * 0.85))),
                    tgtScale*a2rad(node.parent.amount)*-1 + hw*0.15 // minimum visible part
                ) + hw;
            }

PhilippFS commented Sep 1, 2012

For those of you without programming skills, here's the solution in code. Thanks a lot, tgillet1!

            if (node.right==undefined) {
                rad2 = 0 - Math.max(
                    //hw *0.8 - tgtScale * (a2rad(node.parent.amount)+a2rad(node.amount)), // maximum visible part
                    hw * 0.8 - tgtScale * (a2rad(node.parent.amount) + a2rad(Math.max(node.amount*1.15 + node.maxChildAmount*1.15, node.left.amount * 0.85))),
                    tgtScale*a2rad(node.parent.amount)*-1 + hw*0.15 // minimum visible part
                ) + hw;
            }
            else {
            rad2 = 0 - Math.max(
                    //hw *0.8 - tgtScale * (a2rad(node.parent.amount)+a2rad(node.amount)), // maximum visible part
                    hw * 0.8 - tgtScale * (a2rad(node.parent.amount) + a2rad(Math.max(node.amount*1.15 + node.maxChildAmount*1.15, node.left.amount * 0.85, node.right.amount * 0.85))),
                    tgtScale*a2rad(node.parent.amount)*-1 + hw*0.15 // minimum visible part
                ) + hw;
            }
@mrgcohen

This comment has been minimized.

Show comment
Hide comment
@mrgcohen

mrgcohen Sep 7, 2012

works great so far. thanks

mrgcohen commented Sep 7, 2012

works great so far. thanks

@fedossov

This comment has been minimized.

Show comment
Hide comment
@fedossov

fedossov Sep 27, 2012

Thank you very much!

fedossov commented Sep 27, 2012

Thank you very much!

@colinchan

This comment has been minimized.

Show comment
Hide comment
@colinchan

colinchan Mar 21, 2013

Thanks, this really helped!

colinchan commented Mar 21, 2013

Thanks, this really helped!

@Floppy Floppy referenced a pull request that will close this issue Apr 23, 2013

Open

Handle undefined left and right nodes for small trees #24

@Floppy

This comment has been minimized.

Show comment
Hide comment
@Floppy

Floppy Apr 23, 2013

I just ran into this same problem, and also spent a while thinking I was doing it wrong! I've done a slightly different fix that handles both undefined left and right nodes, and opened a PR. Works for me (TM).

Floppy commented Apr 23, 2013

I just ran into this same problem, and also spent a while thinking I was doing it wrong! I've done a slightly different fix that handles both undefined left and right nodes, and opened a PR. Works for me (TM).

@crates

This comment has been minimized.

Show comment
Hide comment
@crates

crates Jun 30, 2014

Floppy's fix is a little more elegant: less lines of code, and works for more use cases. I'll add it here for reference.

    rad2 = 0 - Math.max(
      hw * 0.8 - tgtScale * (a2rad(node.parent.amount) + a2rad(Math.max(node.amount*1.15 + node.maxChildAmount*1.15, (node.left ? node.left.amount : 0) * 0.85, (node.right ? node.right.amount : 0)  * 0.85))),
      tgtScale*a2rad(node.parent.amount)*-1 + hw*0.15 // minimum visible part
    ) + hw;

crates commented Jun 30, 2014

Floppy's fix is a little more elegant: less lines of code, and works for more use cases. I'll add it here for reference.

    rad2 = 0 - Math.max(
      hw * 0.8 - tgtScale * (a2rad(node.parent.amount) + a2rad(Math.max(node.amount*1.15 + node.maxChildAmount*1.15, (node.left ? node.left.amount : 0) * 0.85, (node.right ? node.right.amount : 0)  * 0.85))),
      tgtScale*a2rad(node.parent.amount)*-1 + hw*0.15 // minimum visible part
    ) + hw;

@pwalsh pwalsh added the bug label Dec 15, 2015

@pwalsh pwalsh added this to the Backlog milestone Dec 15, 2015

@pwalsh pwalsh added the help wanted label Dec 15, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment