Fix Sapling Add-on Division by Zero errors
AbandonedPublic
Actions

Authored by Bastien Montagne (mont29) on Nov 5 2014, 3:14 AM.

Details

Reviewers

mangostaniko

Summary

this should fix all remaining division by zero errors in the Sapling Add-on.

to make things easer, the slider min value of baseSize (ratio of tree where there are no branches) has been changed from 0.0 to 0.001, in the belief that this should not be a limitation to the user (not visible even at huge scales). (avoiding baseSize being 0 makes p.lengthPar > 0 which otherwise would cause a lot of vulnerabilites).

Diff Detail

Event Timeline

mangostaniko updated this revision to Diff 2827.Nov 5 2014, 3:14 AM

mangostaniko retitled this revision from to Fix Sapling Add-on Division by Zero errors.

mangostaniko updated this object.

mangostaniko added a reviewer: Bastien Montagne (mont29).

mangostaniko added a project: Add-ons (Community).

mangostaniko added subscribers: Matt Lindley (MattLindley), Philipp Oeser (lichtwerk).

This comment was removed by mangostaniko.

mangostaniko updated this object.Nov 5 2014, 3:15 AM

Thanks for the patch,

have some questions about a few changes there (would not pretend I followed all this spaghetti code, sorry if those are stupid remarks ;) ).

add_curve_sapling/utils.py
571	Why not rather `max(p.lengthPar - baseSize * scaleVal, 1e-6)` ? Would sound safer to me...
595	Why remove safecheck here?
602	Same as comment line571
685	Same as comment line571
688	Why uncomment? Please add inline comments in those cases.

This revision now requires changes to proceed.Nov 7 2014, 6:32 PM

thank you for the comments!
and sorry for not having added inline comments myself before review! i am just learning about phabricator.
hope i answered your questions sufficiently, i realized some mistakes on my part.

aside i have a general question about phabricator Diffs: should i add the people who formerly showed interest in this topic (Matt, Philipp) as subscribers or is that considered rude? should i rather mention this in the former discussion?

add_curve_sapling/utils.py
571	because the whole thing can intentionally become <0 i thought it should not be constraint to 1e-6 as minimum. the thing that was going through my mind was that formerly baseSize was to bet set by the user in [0, 1], and that p.lengthPar can depend on baseSize in some cases even be baseSize, so if the user sets baseSize = 0 we get 0. HOWEVER i changed baseSize to be in [1e-6, 1], since there were many similar issues, but now this is actually not an issue. another thing is that `scaleVal = scale + uniform(-scaleV,scaleV)` so the chances of the whole thing becoming zero are really low. however if scaleV = 0 and baseSize = 1 if the user slides over scale it might get 0. i am really unsure maybe one might just do a max/min branch. i am not even sure if its necessary since the chances are so low... (is that a terrible argument?)
595	p.lengthPar can only be 0 if baseSize = 0, and since baseSize was changed from [0, 1] to [1e-6, 1] this can no longer happen. i changed the baseSize interval since there were so many situations like this one.
602	answer is also the same
685	yes this should be (scaleVal * max(1 - baseSize, 1e-6)). scaleVal can not be 0, but baseSize is in [1e-6, 1] and if its 1 we get 0. my 'solution' is actually terrible, since when baseSize is 1-(1e-6) we are in trouble. if we change the shift to - 1e-6 then we have problems at baseSize minimum. sorry i really seem to have rushed that too much :/
688	just passing it leaves insideBool unassigned which can cause an UnboundLocalError later (tested). you mean phabricator inline comments? sorry for that i am just learning about the whole process :) or do you mean in the code?

Thanks for the answers, they make sense to me. Please update the patch with corrected code where needed, then.

And you can always add as CCs people who you think might be interested by your patch - if they really do no want to be poked, they can always remove themselves from CCs list.

add_curve_sapling/utils.py
688	Ok, think it should be OK then (and I meant comment in code, yes, something like `# need to init insideBool here, else we can have UnboundLocalError later.`)

incorporated suggested changes

also, baseSize value has been further restricted from minimum 1e-6 to minimum 0.001. should not be a limitation to the user as this is not visible even at really huge scales. raising the value avoids problems with rounding errors in extreme cases when multiplied with other extremely small values, like branch length 1e-6.

this solves the issues at 571 and 602, which did occur at extreme conditions as explained before, without needing to add 1e-6.

Ok, seems good. Do you have a (public) mail address, to use in git commit, so that you are seen as author of it?

nikolaus.leopold on gmail
should i commit locally and create a pull request? .. no wait thats not github..
so i guess you can do it?