Horrible hack to work around bug in OCCT7.7.2 #15931
Conversation
bgbsww
force-pushed
the
bgbsww-toponamingFixBug15599
branch
from
346cade
to
72f00a6
Compare
| @@ -665,7 672,7 @@ App::DocumentObjectExecReturn *Cone::execute() | |||
| BRepPrimAPI_MakeCone mkCone(Radius1.getValue(), | |||
| Radius2.getValue(), | |||
| Height.getValue(), | |||
| Angle.getValue()/180.0f*M_PI); | |||
| angle/180.0f*M_PI); | |||
There was a problem hiding this comment.
nitpick: maybe use Base::toRadians, as elsewhere in this file?
There was a problem hiding this comment.
Absolutely a legitimate point, but M_PI / 180 appears about 68 times, and 180 / M_PI appears 10 times, and 180 * M_PI appears 9 times and 180.0f*M_PI appears 9 times (8 in this very file) ... you get the point. I think that should be somebody's separate PR to clean up all the angle/radian conversions, and not part of this extremely narrowly focused piece of nastiness, since there isn't a clear line on where to stop. Just this one? This one and the one in the clause above it? All of them in this file? All of them in App?
There was a problem hiding this comment.
I see your point and I agree that this very OCCT workaround should not be mixed with general cleanup.
However, if I touch a line of code anyway, I can "touch it for the better". -- Your mileage may vary and I accept that.
There was a problem hiding this comment.
You are quite right, and I appreciate the review. I value consistency over prettiness, and since this specific improvement would work against consistency unless at a minimum the other M_PI usage in the same method were changed, that means (to me) that "better" is relative. Again, I do think that an en-mass change of all of these, or at least a file-by-file change of all of these makes sense, and I might even do it - just not as part of this one very narrow PR. There are a lot of M_PI in combination with constants of 2, 3 ,4 ,12 and 180 that look ripe for improvement to me, and at the very least, not enough M_PI2. This conversation is more about what principles apply and make sense and not the specific change, and I'm glad you triggered it. As you point out, it is a varying perspectives kind of thing. Thanks! I could definitely see a non-critical bug filed for "Refactor PI times a constant math expressions out of the source code". In fact, the general category of code beautification bugs makes sense to me.
There was a problem hiding this comment.
@jbaehr can you make an Issue for that? We can discuss in tomorrow's review meeting, but I'm inclined to merge this as-is and then promote a bigger-picture refactor to make the suggested change.
|
Such ugly workarounds should really be avoided because they don't fix anything but move problems from A to B. Only because a cone with an angle of 359.99 seems to work here doesn't mean that the cone feature (or the BRepPrimAPI_MakeCone algorithm, respectively) is to blame. After all it's still the boolean operation that causes the problem. Now the first regressions of this workaround can be seen here: |
|
Do you feel we should revert this one, @wwmayer ? |
|
Yes. The problem reported in #15599. is not because of a bug in the cone creation but in the boolean operation. And when reading further this problem doesn't only occur with OCC 7.7.2 but also with 7.8.1. And as Syres916 correctly figured out it's a problem with co-planar faces. And this is a general problem with OCC we very frequently see with boolean operations. Btw , a much better workaround than using an angle of 359.99 is to rotate the cone around its main axis. It suffices to rotate it a tiny bit but I recommend to rotate it by 90 deg because then the seam edge is not so visible. And the nice side-effect is that even the geometry checker doesn't find any defects in the shape any more while with this PR it still finds defects. |
|
Here is an interesting reading about the failure of boolean operations: https://wiki.mcneel.com/rhino/booleanfaq |
|
OCC can't merge coplanar face but can fill a hole between them (see fuzzy value) so we could create a hole. EDIT: @wwmayer OCC have interesting doc too: https://dev.opencascade.org/doc/overview/html/specification__boolean_operations.html#autotoc_md293 |
That's a very interesting document!! We should link that in our FAQ somewhere... Ok I'll prepare a PR to revert this one.
Can you make a PR for that? Otherwise I can try too, but not sure where it should be done... Not at cone creation ,right? |
This isn't something that should be coded but it must be done by the user. As explained in the Rhino FAQ the user must be aware of the problems with boolean operations and he has to help the kernel to do its job right. |
|
Ok I see... You mean if there are problems with coinciding faces/edges. the user should move something, change something or as in your example, rotate the cone slightly. But it's not us who should arbitrarily rotate a cone to hypothetically prevent a possible boolean problem. Indeed that makes sense to me too. In most other cases which don't involve booleans, the user will expect the topology to be correctly aligned by default. |
Fix #15599.
The failure is actually in cone, and represents a bug in 7.7.2 but not 7.6.3 and not 7.8.x. Angle values of 360.0 fail completely, 359.9999 and 359.999 partially, and 359.99 results in an ugly fix.
If it isn't clear, I hate this code, but it seems like the pragmatic best choice; the alternative is FreeCADs with 7.7.2 don't work with cones of 360 degrees. I'm gonna take a bath now.