FIX bumpy flatmap smoothing skipped on the left hemisphere - #678
Open
mvdoc wants to merge 1 commit into
Open
Conversation
`smoothfactor` and `smoothiter` were declared with `var` inside the `if (this.flatlims !== undefined)` block but used earlier in the same per-hemisphere loop, to smooth `wmareas` and `pialareas`. Hoisting made the names visible there but left them `undefined` on the first iteration, so `iterativelySmoothVertexData` ran `for (i = 0; i < undefined; i++)` -- zero iterations -- and returned the areas unsmoothed. The second iteration then saw the values assigned during the first, so the left hemisphere was unsmoothed and the right was smoothed, giving the two hemispheres visibly different bump relief. Declare the parameters above the loop, split into separate constants for the area and distance passes, and reduce the iteration counts (areas 50 -> 5, dists 50 -> 20) for a somewhat less smoothed result. Split out of #310. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Splits the bug fix out of #310 (which has been sitting since 2019 and no longer merges cleanly).
The bug
In
mriview_surface.js,smoothfactorandsmoothiterare declared withvarinside theif (this.flatlims !== undefined)block, but used ~25 lines earlier in the same per-hemisphere loop to smoothwmareasandpialareas.varhoists the declaration to function scope but not the assignment, so on the first iteration of the loop they are stillundefined.iterativelySmoothVertexDatathen runs:0 < undefinedisfalse, so the loop body never executes and the areas come back completely unsmoothed. By the second iteration the values assigned during the first pass are visible, so:which is why the two hemispheres have visibly different bump relief. I confirmed the bug is still present on current
main.The fix
Declare the parameters above the loop, split them into separate constants for the area and distance passes, and reduce the iteration counts (areas 50 → 5, dists 50 → 20) for a somewhat less smoothed result.
The commit adds a comment explaining why the declarations have to live outside the loop, so the bug does not get quietly reintroduced by re-nesting them.
Note for reviewers
The iteration-count reduction is a deliberate appearance change riding along with the correctness fix — it is part of what @alexhuth was going for in #310 ("I also reduced the amount of smoothing somewhat"), but it is separable if you would rather land the hoist alone first. These are also hardcoded constants; they could reasonably be exposed in
[webgl_viewopts]next tobumpy_flatmap.Testing
node --checkpasses. Not verified visually — no render was produced.The lighting half of #310 is split into a separate PR.
🤖 Generated with Claude Code