New shape: Quarterpipe #3960
New shape: Quarterpipe #3960
Conversation
|
Are you sure that this is needed since the addition of Union shapes? |
|
I am pretty sure I cannot combine a |
doc/sphinx/constraints.rst
Outdated
|
|
||
| A smoothed concave corner, which can be used to connect two walls or rhomboids in a smooth way. | ||
| The shape is constructed by removing a quarter cylinder of radius ``radius`` | ||
| from a cuboid with side length ``radius``. It can be created via :: |
There was a problem hiding this comment.
i think you need to add a sketch to make this clear
|
I realize that the |
|
@christophlohrmann you'll have to merge the python branch into the PR to pass CI, all 3 runners are failing (#3895 (comment)). |
src/shapes/src/SmoothCorner.cpp
Outdated
| while (pos_delta_phi > 2 * Utils::pi()) { | ||
| pos_delta_phi -= Utils::pi(); | ||
| } |
There was a problem hiding this comment.
Why do we need to do this? According to std::atan2 the return value should be between [-pi, pi] if nothing goes wrong. If it is necessary, is std::fmod what you are looking for?
There was a problem hiding this comment.
Thanks for pointing that out, I thought the result would be in [0,2 pi]. I will remove the loop
src/shapes/src/SmoothCorner.cpp
Outdated
| if (dist < 0. and dist_top_bottom < 0) { | ||
| if (Utils::abs(dist_top_bottom) < Utils::abs(dist)) { | ||
| dist = dist_top_bottom; | ||
| vec = upper_lower * dist * m_axis; | ||
| } | ||
| } |
There was a problem hiding this comment.
This expression can be evaluated by two comparisons
| if (dist < 0. and dist_top_bottom < 0) { | |
| if (Utils::abs(dist_top_bottom) < Utils::abs(dist)) { | |
| dist = dist_top_bottom; | |
| vec = upper_lower * dist * m_axis; | |
| } | |
| } | |
| if (dist_top_bottom < 0 and dist_top_bottom > dist) { | |
| dist = dist_top_bottom; | |
| vec = upper_lower * dist * m_axis; | |
| } |
|
If I understand the description correctly:
|
|
Is a review of the actual code still needed, or did someone do that already? |
|
|
I have not reviewed it, i just pointed out the things which i found by just briefly looking at the code. |
src/shapes/src/SmoothCorner.cpp
Outdated
| vec = pos_projected - to_point_1; | ||
| dist = vec.norm(); | ||
| } else { | ||
| if (pos_delta_phi >= 0.5 * m_delta_phi) { |
There was a problem hiding this comment.
i guess this can be brought to the outer layer with an else if
i think you just qualified for a full review 👍🏻😜 |
src/shapes/src/SmoothCorner.cpp
Outdated
| * You should have received a copy of the GNU General Public License | ||
| * along with this program. If not, see <http://www.gnu.org/licenses/>. | ||
| */ | ||
| #include <boost/algorithm/clamp.hpp> |
There was a problem hiding this comment.
I see where this is coming from but you don't need this.
| #include <boost/algorithm/clamp.hpp> |
doc/sphinx/constraints.rst
Outdated
| ``axis`` is the axis of the cylinder, | ||
| orthogonal to the curve of the pipe. |
There was a problem hiding this comment.
this linebreak looks odd to me because the line below is even longer
| ``axis`` is the axis of the cylinder, | |
| orthogonal to the curve of the pipe. | |
| ``axis`` is the axis of the cylinder, orthogonal to the curve of the pipe. |
src/shapes/src/Quarterpipe.cpp
Outdated
|
|
||
| auto upper_lower = rel_z > 0 ? 1 : -1; | ||
| // signed distance to the closer top/bottom wall | ||
| auto dist_cover = Utils::abs(rel_z) - 0.5 * m_height; |
There was a problem hiding this comment.
you could replace the Utils:abs call which you have also done in all cases below
| auto dist_cover = Utils::abs(rel_z) - 0.5 * m_height; | |
| auto dist_cover = upper_lower * rel_z - 0.5 * m_height; |
src/shapes/src/Quarterpipe.cpp
Outdated
| bool in_zone_4_or_5 = | ||
| Utils::abs(dist_plane_1) < Utils::abs(dist_plane_2); |
There was a problem hiding this comment.
we already know the sign from the else if above...
| bool in_zone_4_or_5 = | |
| Utils::abs(dist_plane_1) < Utils::abs(dist_plane_2); | |
| bool in_zone_4_or_5 = dist_plane_1 > dist_plane_2; |
src/shapes/src/Quarterpipe.cpp
Outdated
| vec = pos_projected - to_point_2; | ||
| dist = vec.norm(); | ||
| } else { | ||
| if (pos_norm < m_radius) { |
There was a problem hiding this comment.
this can also be brought one layer out as an else if
christophlohrmann
force-pushed
the
smooth_corner
branch
from
4ba164e to
13abdee
Compare
| */ | ||
|
|
||
| #include <shapes/Quarterpipe.hpp> | ||
| #include <utils/Vector.hpp> |
There was a problem hiding this comment.
| #include <utils/Vector.hpp> | |
| #include <utils/Vector.hpp> | |
| #include <utils/constants.hpp> |
src/shapes/src/Quarterpipe.cpp
Outdated
| // Distinguish the Zones | ||
| if (dist_plane_1 >= 0. and dist_plane_2 >= 0.) { | ||
| // Zone 9 | ||
| auto corner_point = std::sqrt(2.) * m_radius * m_orientation; |
There was a problem hiding this comment.
| auto corner_point = std::sqrt(2.) * m_radius * m_orientation; | |
| auto corner_point = Utils::sqrt_2() * m_radius * m_orientation; |
| shape=espressomd.shapes.Quarterpipe(center=[25, 15, 15], axis=[1, 0, 0], | ||
| orientation=[0, 1, 1], radius=25, | ||
| height=30), | ||
| particle_type=0, penetrable=True) |
There was a problem hiding this comment.
- I'm not sure it is a good idea to have two sharp edges in a shape interacting with a LJ liquid; wouldn't it be safer to have e.g. 4 quaterpipes to form a pipe, or a quaterpipe with its X,Y,Z edge lengths equal to the box lengths?
- the rasterization doesn't draw dots on the shape top and bottom surfaces, giving the impression the shape is hollow.
There was a problem hiding this comment.
the shape becomes unsafe when side walls touch the periodic boundaries. Then Particles can enter the interaction region from the other side and pick up a lot of potential energy that propels them through the box. I will change the height though to hide the top/bottom
|
What is the status? Is there a conclusion about the sketch? |
review in progress |
|
decided to extend the existing HollowConicalFrustum by a orientation and central angle argument. |
|
* `decided to extend the existing HollowConicalFrustum by a orientation and central angle argument.`
How can a Kegelstumpf be made a Quaterpipe?
|
|
the cone with 0 conicity is a finite cylinder. if you further allow to specify the central angle you end up with a quarter pipe |
|
OK. That such a creative re-use is possible would have to be made clear in the documentation including an image, I think.
The name of the shape doesn’t really suggest it. In theory, a new name would also be possible, since the shape was not in a release yet.
|
|
it's just a segment of the existing shape, thats not too far fetched |
|
See #4014 |
|
this will be implemented by recycling the |
Description of changes: