LatheGeometry: Improve normal generation. #22927
Conversation
Two-step approach: 1. pre-compute normals in 2D-domain (all z = 0) for initial "meridian" of vertices as defined in the points[] array 2. rotate pre-computed normals along with vertices for each additional meridian in lathing Advantages: * Considerably reduces computational expense of setting up normals. * perfect normal precision (within limits of numerical accuracy), avoids considerable deviations from precision in low lathe-segment count situations of current approach * seam-handling obsoleted by design This is going to be my first pull request. I hope, I'll be able to add screenshots/scetches later in the process.
Struggling with my 1st PR and now fixing previously ommitted code lines.
Fixed linting errors
|
I renamed the branch to give it a more descriptive name. Apparently this implicitely closed the pull request. |
|
Guys,
please be patient - this is my 1st PR and I’m struggling on all fronts which are not 3D-related (git and Github in particular). If anyone sees the need to change branch assignments, please do so as necessary. Or please provide some guidance on how I should do it properly.
I believe my proposed approach has the potential to eliminate seam handling once and for all, and can probably be applied to several more rotationally symmetrical geometries as well (think of cone, cylinder, torus, sphere ...).
Thanks,
Chris
… Am 01.12.2021 um 15:44 schrieb WestLangley ***@***.***>:
Related: see the discussion in #8334 <#8334>.
Also, if @Mugen87 <https://github.com/Mugen87> approves this PR, it needs to target the dev branch -- not master.
—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub <#22927 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ASCFLXAAELIPJMBMLVKILPLUOYYDVANCNFSM5JEBMKSA>.
Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
I'll fix this 👍 . |
|
btw: the requirement for each x-coordinate to be non-zero would also be obsoleted. Because in 2D, normals computation does not involve a division. |
|
@Chrissie-AI would be nice to try and weight the normals by segment lengths, to avoid the situation where minor bump affects the normals of the large area |
@makc, I'm absolutely in favour of your proposal and frankly: I thought I had already implemented it this way in my proposal, by building the vectorial sum of two adjacent raw (i.e.: not yet normalized) normals and delaying the normalization of the resulting pre-computed normals until after building the average resulting normal per vertex. If I failed to do so, which is entirely possible, could you please point me to the code line where I missed something? As a side note, regarding the same subject: averaging by path-segment length is a one-dimensional thing, while the previous/current method of averaging the cross-product of two in-face vectors of participating faces is more of a face-size related thing, which would be two-dimensional. While I do expect the two methods of averaging to yield different results, I also expect the averaging by path-segment length to be physically closer to what we are trying to visualize. If you view this differently, please provide a proposal of what you would like to see instead. |
|
@Chrissie-AI ah disregard this, I see that dx/dy are not normalized before this, so it is indeed as you say
my bad |
|
no worries. Since I had already prepared my answer, I attach the following sketch I already made for the posterity: |
|
Current implementation: https://jsfiddle.net/t4hf51zn/1/ The PR indeed improves the normal generation. I also like the fact that there is no special handling for closed geometries anymore. |
Mugen87
changed the title
|
Thanks! |
This proposed change follows a two-step approach:
Path: blue; Vertices on path: red; axis of lathe: green; Normals: red.
Advantages:
This is going to be my first pull request. Please see attached screenshots for clarification of the concept.
Related issue: #XXXX
Description
I observed significant mis-alignment of normals computed via computeVertexNormals() from their expected true orientation, especially in low lathe segment counts. Those mis-alignments have considerable impact on the quality and fidelity of computed reflections.
I found the cause for the observed mis-alignments to be a lack of symmetry in the faces surrounding a shared vertex. The lack of symmetry may manifest itself in an unequal number of faces "pulling" left vs. right, or up vs. down, or different sizes of neighbouring faces sharing the same vertex. Consider for instance the following 4-segment, full circle lathe of a 2-vertex path:
points.length = 0;
points.push( new THREE.Vector2( 15.0, 0.0 ) );
points.push( new THREE.Vector2( 5.0, 10.0 ) );
Looking straight down the Y-axis, seam at bottom meridian:
Vertex number 8 (counting from 1 to 8), circled green, is a case in point: two adjacent faces on the south-west slope pulling down, only one face on the north-west slope pulling up. Hence we observe a systematic CCW twist of the projected normal @ all inner vertices away from the expected radial orientation - except for the seam, which looks pretty much OK (but still isn't, if you look at the numeric components of the normal @ vertex number 2.
Likewise, for the same reason, we observe a systematic CW twist of the projected normals @ all outer vertices away from the expected radial orientation - again except for the seam.
My proposed change yields perfect radial alignment of the projected (along the axis of lathing) normals and makes special seam handling obsolete by design.
Please note, that fixing the mis-aligned normals of the current approach may break some E2E test. This is in itself would not be indicative of an error.