acts
acts copied to clipboard
acts-project
Bug: Probable Issues in Bevelled Cylinder Implementation
The issue is a summary of the probable problems with PR 1104 which implemented the bevelled cylinder surfaces in ACTS. Slide 8, 9 here.
In Acts::CylinderBounds::inside:
- The vector lposition seems to represent a location in the (Rphi, z) coordinates. This would make sense as a cylindrical surface would unwrap to be a rectangle with the same height as the cylinder but a width = 2piR. The calculation of the localx variable and its use localx does not seem to reflect that. The factor of radial location R is missing.
- The first check in Acts::CylinderBounds::inside is performed on a shifted lposition variable which normalizes out the radial factor. This check does not check for any bevels and seems like it would return true even without checking edge cases for a bevelled cylinder. So, the rest of the code block never gets executed.
- The block of code following line 62 and line 65, seems to assume that the boundary of a unwrapped bevelled cylinder is a polygon. This wouldn't be the exact case if one takes into account the evolution of the slope along the boundary surfaces.
In Acts::CylinderBounds::inside3D:
- The inside3D function has two checks: a fast check that does bevelled cylinders but with full azimuthal range (closed surface) and a slow check that can handle phi segments but doesn't check for bevels. Does the fast check need to be updated to check for finite azimuthal range or is the check here sufficient?
- In the fast check, should this be bevelMaxZ? Seems like a typo.
- In the fast check, it seems to again only check a projection on the yz-plane and may not be sufficient for a bevelled cylinder with finite azimuthal range.
A better drawing of the issue constructed in onshape. In the top diagram, a cylinder with a bevelled shape is constructed and then unwrapped. The circumference of a cylinder is given by 2* pi* R if the radius is R. Since the radius of the bevelled cylinder shown in the top diagram is 51.24 mm, the peak height in the unwrapped plane occurs at pi*51.24/2=80.49 or quarter distance along the circumference. Starting from there, we calculate the height at each point along the circumference and the unwrapped shape shows a sign wave formation at the top edge. The unwrapped plane is not a polygon as assumed in the existing implementation.
Propose updating the unit test in the following manner. Construct the nominal example in onshape. Instead of a single point, grab the coordinates of multiple points along the circumference with the cut plane at the exact position and then check for all of those in the unit test instead of just a single point. Move the cut plane up and down in z beyond a certain tolerance and again add checks for points along the circumference. May be 4-5 points per case should be sufficient. I will make a WIP for this if it sounds good enough .
This issue/PR has been automatically marked as stale because it has not had recent activity. The stale label will be removed if any interaction occurs.
![stale[bot] avatar](https://avatars.githubusercontent.com/in/1724?v=4 "Published by a pub.dev verified publisher"){kind=small}
This issue/PR has been automatically marked as stale because it has not had recent activity. The stale label will be removed if any interaction occurs.
The boundary condition is implemented as sinusoidal but the unit tests still need to be updated here .
This issue/PR has been automatically marked as stale because it has not had recent activity. The stale label will be removed if any interaction occurs.
This issue/PR has been automatically marked as stale because it has not had recent activity. The stale label will be removed if any interaction occurs.
This issue/PR has been automatically marked as stale because it has not had recent activity. The stale label will be removed if any interaction occurs.