compute proper bounding box for cylinders #15
Conversation
|
Hi, sorry for this very late answer. |
|
No worries! Seems that I'm a bit late to respond as well. I've created an openems automatic mesh generation tool (still a work in progress) that uses the bounding box of each object as the basis for determining the position of mesh lines. There are cases in which I need a cylinder to terminate in a PML at the outer face of a simulation structure. Having the bounding box for cylinders extend beyond their actual boundary makes my tool think that the structure extends further out and creates mesh lines there, preventing the PML from working. Here is an example of a CSXCAD structure exhibiting the intended behavior (using this PR), and here is an example with the unintended behavior (unfortunately, github won't let me upload XML files). I've also attached images of the correct (first) and incorrect (second) structures (ignore the very non-smooth mesh lines in the incorrect structure, this case isn't supported by my tool). If it isn't clear from the images, the first image differs from the second in that mesh lines extend beyond the boundary of the coaxial cable in the second image, whereas they terminate at the boundary in the first image.
Let me know if you need more information or if this post isn't clear in some way. |
|
Hm I have picked you commit, but somehow github does not really notice? |
|
One more comment: I think the implementation in "CSPrimCylindricalShell" and "CSPrimCylinder" is pretty redundant? Maybe put it in a special funtion to reuse in both cases??? |
|
I have removed the redundancy. Only change I can see: CSPrimCylindricalShell now also claims to be "accurate", but I would not see why it should not while the CSPrimCylinder should? I think we can close this PR? |
|
Looks good, thanks! I'm not sure why I just copied that function definition in two places... In any event, how you have it now is much better. |
|
Unfortunately I found a major issue with your commit... You make the "orientation" of the bounding box dependent on if the cylinder axis start<=stop or not... But the BB must always be oriented start<=stop for all directions Note: Please use only tabs not spaces! I missed that too... |
Can you clarify the intended behavior? I was under the impression that start[i] must be <= stop[i] (i.e., for each coordinate). It's been quite a while since I wrote this code, but from a quick glance (I'll take a more detailed look soon) it looks like that's what the "flip" logic ensures. Again, I can take a closer look and verify this if you confirm the behavior it should exhibit.
Sorry about that, I can create a PR with the spaces replaced with tabs, or you can, whichever you prefer. |
|
I'm already in the process to understand it and have already replaced the spaces (no big deal!) |
|
BTW: I found it when I investigated for #27 |
|
That looks correct to me. Sorry about the bug and thanks for the fix! |
This incorrect behavior corrected for a bug I introduced in thliebig/CSXCAD#15. Thorsten identified and fixed that bug, which causes this code to produce incorrect results.
This PR computes a proper bounding box for cylinders and cylindrical shells. It does this by computing parametric equations for the terminal circles and then computing maxima and minima (parametric circle equations use the method described here).
Let me know what you think of this. Happy to make any/all changes needed. In particular, this won't handle zero-height cylinders correctly, although I'm not sure what the intended behavior is there. If you do have specific behavior in mind for that, let me know and I'll make this work with it.
Additionally, I'm not sure how to get the bounding box of transformed cylinders. Normally, I get the original bounding box of the object and then transform each coordinate, but that doesn't seem to work here. Any thoughts on how to do this?