Generalised cylinder #86
Conversation
There was a problem hiding this comment.
Some fixes needed.
I haven't reviewed the whole body. I don't want to receive it in the same PR. Note whole body doesn't work for me as missing torso terms file?
I also need to spend more time reviewing cylindermesh, but would like some fixes done first.
Generation was slow - found source due to 2 calls to Fieldmodule beginChange, but one endChange. You don't need these in scaffold any more, but may want in utilities.
| 'Refine number of elements along' : 1, | ||
| 'Refine number of elements across' : 1, | ||
| 'Refine number of elements up' : 1 |
There was a problem hiding this comment.
You can't actually have a different refine number of elements across the major and minor directions due to the triple point elements. Hence remove one of these. In ventricles3 I use 'surface', but perhaps 'section' makes more sense here?
Values seem to cause crashes for me - has this been tested?
There was a problem hiding this comment.
Mesh refinement fixed and tested. I did not understand the 'surface and section' part.
There was a problem hiding this comment.
Mesh refinement is working now and tested. What do you mean by " In ventricles3 I use 'surface', but perhaps 'section' makes more sense here"?
| CYLIDNER_REGULAR = 1 # all the bases along the axis of the cylinder are the same. | ||
| CYLIDNER_TRUNCATED_CONE = 2 # different ellipses along the cylinder axis |
There was a problem hiding this comment.
Enum names and descriptions are not helpful. What are bases? Which ellipses are being referred to?
Typo: CYLIDNER
There was a problem hiding this comment.
I used STRAIGHT and TAPERED instead if that makes more sense?
| elementsCountAcross, elementsCountUp, elementsCountAlong, | ||
| cylinderMode = CylinderMode.CYLINDER_MODE_FULL, | ||
| cylinderType = CylinderType.CYLIDNER_REGULAR, | ||
| rate = 0.0, progressionMode=ConeBaseProgression.GEOMETIRC_PROGRESSION, useCrossDerivatives = False): |
There was a problem hiding this comment.
progressionMode is not documented as :param:. Why isn't there separate control on major, minor.
I'm wondering if instead of loading up arguments here that you provide a central path with major/minor radius fields?
There was a problem hiding this comment.
What do you mean "provide a central path with major/minor radius fields"?
There was a problem hiding this comment.
I defined a class to store these parameters in it. I called it TAPERED and I store major and minor common ratio and progression type (r_n+1=r_n+ratio for arithmetic and r_n+1=r_n*ratio for geometric progression). In case of geometric sequence "ratio" makes sense but for arithmetic it should be common difference. I call it ratio and there is a comment to explain this. Do you have any better terms for TAPERED class or ratio?
change and fix suggested changes
I would like to add a new utility for generating a generalised cylinder (with ellipse/circle base, could be a truncated cone as well) scaffold, please.