-
Notifications
You must be signed in to change notification settings - Fork 35
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add solid core to tubenetwork mesh #262
Add solid core to tubenetwork mesh #262
Conversation
oneway1225
commented
Jul 29, 2024
- Add solid core component to tubenetwork mesh.
- Add 3D whole body scaffold
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the review for everything but tubenetworkmesh.py, which I'll continue after.
These are some simple fixes which can be completed today, and I may be able to get the code in with them.
if self._isCore and self._elementsCountTransition > 1: | ||
self._elementsCountAround = (elementsCountAround - 8 * (self._elementsCountTransition - 1)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be checked and adjusted in the scaffolds' checkOptions method.
Elements count around should be satisfied first and transition/across major adjusted to fit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason why I had this in the tubenetworkmesh instead of the scaffolds' checkOptions is because I couldn't figure out a nice way of switching between one transition element case and multiple transition elements case.
For example, if I had an initial elementsCountAround of 16 in the UI, and I change elementsCountTransition to 2, then the new elementsCountAround would be 8. If this change was made in the UI, then the values that get sent to tubenetworkmesh would be 8 and 2. Now, if I change the elementsCountTransition back to 1, then the values that get sent to tubenetworkmesh would be 8 and 1, which is different to the initial elementsCountAround of 16.
Any suggestions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just need a priority e.g. number around > transition > across major.
I think it's best to make the number around guaranteed, then work with the transition ensuring its possible for the number around or reducing to 1, then fix the across-major number to work in with the others.
P0 = x[self._elementsCountAround // 4] | ||
P1 = x[self._elementsCountAround // 4 * 3] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Requires multiple of 4 around. Safer to do mean of all points around.
|
||
coreCentres, arcCentres = [], [] | ||
for i in range(self._elementsCountAround): | ||
tol = 1e-10 # tolerance to avoid float division zero |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tolerances should not be absolute values. Can determine max range from outer coordinates and take 1.e-10 x that.
OP = magnitude(sub(ox[i], cp[0])) | ||
IP = magnitude(sub(ix[i], cp[1])) | ||
distBetweenOuterAndInner = magnitude(sub(cp[1], cp[0])) | ||
distBetweenOuterAndInner = tol if distBetweenOuterAndInner == 0 else distBetweenOuterAndInner |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be more logical to use max(tol, distanceBetweenOuterAndInner)
.
IP = magnitude(sub(ix[i], cp[1])) | ||
distBetweenOuterAndInner = magnitude(sub(cp[1], cp[0])) | ||
distBetweenOuterAndInner = tol if distBetweenOuterAndInner == 0 else distBetweenOuterAndInner | ||
outerBase = (OP ** 2 - IP ** 2 - distBetweenOuterAndInner ** 2) / (2 * distBetweenOuterAndInner) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The algorithm is non-obvious, so I'd want a comment explaining it in plain english.
tests/test_wholebody2.py
Outdated
region = context.getDefaultRegion() | ||
self.assertTrue(region.isValid()) | ||
annotationGroups = scaffold.generateMesh(region, options)[0] | ||
self.assertEqual(5, len(annotationGroups)) # Needs updating as we add more annotation groups |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs fixing, see new comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Accepted, but note some node sampling issues need to be fixed up in subsequent PRs.
cbx, cbd1, cd2, cbd3, ctx, ctd1, ctd2, ctd3 = [], [], [], [], [], [], [], [] | ||
|
||
for i in range(m): | ||
for lst in (cbx, cbd1, cd2, cbd3): | ||
lst.append([[0, 0, 0] for _ in range(n)]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cd2 misnamed and not used, ctd2 not used
tmdd3 = id3[startIndex] | ||
rcx = [tmdxStart, tmdxEnd] | ||
mag = -1 if n == 0 else 1 | ||
rcd3 = [set_magnitude(tmdd3, mag), set_magnitude(tmdd3, mag)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The middle derivative should surely be closer to difference between end and start coordinates.
Result with this code will be a symmetric cubic wave with a peak in the middle. Can't see how the current geometry is created with this.
This replacement code fixes boundary glitches caused by the above issue:
tmdd3Delta = sub(ix[n2z], ix[n2a])
for n in range(2):
startIndex = [n2a, n2z][n]
tmdxStart = ix[startIndex] if n == 0 else centre
tmdxEnd = centre if n == 0 else ix[startIndex]
rcx = [tmdxStart, tmdxEnd]
tmdd3Start = [-d for d in id3[startIndex]] if n == 0 else tmdd3Delta
tmdd3End = tmdd3Delta if n == 0 else id3[startIndex]
rcd3 = [normalize(tmdd3Start), normalize(tmdd3End)]
Note the issue only affected the first and last box values on the minor rows as the remaining values are resampled for the major columns.
for n1 in range(self._elementsCountAround // 4 * 3 - nSkip, | ||
self._elementsCountAround // 4 * 3 + nSkip + 1): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Define separate skip number for the major axis:
oSkip = self._elementsCountAcrossMajor // 2 - (1 + self._elementsCountTransition)
Need to use that to fix the other direction; logically that should have happened in _sampleCoreNodesAlongMinorAxis.
for n1 in range(self._elementsCountAround // 4 * 3 - oSkip,
self._elementsCountAround // 4 * 3 + oSkip + 1):