Add --flip-normals to vc_render_tifxyz (issue #1044)#1079
Open
robert-kofler42 wants to merge 1 commit into
Open
Add --flip-normals to vc_render_tifxyz (issue #1044)#1079robert-kofler42 wants to merge 1 commit into
robert-kofler42 wants to merge 1 commit into
Conversation
grid_normal computes surface normals with the right-handed convention N = dP/dU x dP/dV. When the segment's text is readable (not flipped in UV space), that normal points away from the scroll centre, so the offset/w direction is the opposite of what's usually wanted. This recurs often enough to bite people (ScrollPrize#1044). Add a core helper flip_surface_normals(normals) that negates each normal in place while leaving NaN-sentinel (invalid) normals untouched, and a --flip-normals switch to vc_render_tifxyz that applies it in the single normal-processing chokepoint (prepareBaseAndDirs, used by all render paths). Off by default, so existing behavior is unchanged. This is the lightweight per-render option the issue asks for; it does not change grid_normals globally or attempt auto-orientation (which is scroll/handedness dependent, per the issue) — those remain future work. Adds test_flip_surface_normals covering negation, double-flip identity, NaN-sentinel preservation, and the empty-matrix case. vc_render_tifxyz compiles with the new option.
robert-kofler42
requested review from
SuperOptimizer and
hendrikschilling
as code owners
|
Someone is attempting to deploy a commit to the scroll Team on Vercel. A member of the Team first needs to authorize it. |
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
1 participant
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.
Why
Closes part of #1044.
grid_normalcomputes surface normals with the right-handed conventionN = dP/dU × dP/dV. When a segment's text is readable (not flipped in UV space), that normal points away from the scroll centre, so the offset/wsampling direction is the opposite of what's usually wanted. Per the issue this "comes to bite us ever so often," and the reporter notes they'd "just add a flag in vc_render_tifxyz to flip the normals, which I will always enable."What
flip_surface_normals(cv::Mat_<cv::Vec3f>&)inGeometry— negates each normal in place, leaving NaN-sentinel (invalid) normals untouched so validity is preserved.--flip-normalsswitch invc_render_tifxyz, applied at the single normal-processing chokepoint (prepareBaseAndDirs, used by all three render paths). Off by default → existing behavior unchanged.Scope is deliberately the lightweight per-render option the issue asks for. It does not change
grid_normalsglobally or attempt auto-orientation — that's scroll/handedness-dependent (the issue itself flags the exceptions, see scrollprize.org/faq#/scroll-2) and is better decided by the team; left as future work.Verification
test_flip_surface_normals(4 cases): negation of finite normals, double-flip identity, NaN-sentinel preservation, empty-matrix no-op — all pass.vc_render_tifxyzcompiles with the new option wired through.