Fix projective rendering for image source quads - #7887
Conversation
01ac786 to
42d5993
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7887 +/- ##
==========================================
+ Coverage 92.79% 93.09% +0.29%
==========================================
Files 300 300
Lines 24904 24898 -6
Branches 6528 6540 +12
==========================================
+ Hits 23109 23178 +69
+ Misses 1795 1720 -75 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Thanks for taking the time to open this PR, please make sure to fill the PR template. |
|
Thanks, filled in the template now. |
There was a problem hiding this comment.
honestly I think that the grid image that you have is likely a better for the render tests.
Should we add it to that grid image here instead of the beach?
There was a problem hiding this comment.
I'm a bit confused from the before and after image here, is this a problem that was intorduced as part of this PR as I wouldn't expect this change - resampling should show pixelated image, and that's not the case here.
Am I missing anything?
There was a problem hiding this comment.
I guess yes, for resampling the non-projection is maybe better.
We somewhat care about both projections and reasampling though, so lets revert this, that was a bad suggestion for this one.
There was a problem hiding this comment.
Restored this fixture to local://image/0.png and regenerated its expected output, so it continues to cover nearest-neighbor resampling. The grid image is now limited to the new projective-rendering fixtures.
There was a problem hiding this comment.
I still expect this not to change, can you provide an explanation on why the image had changed?
There was a problem hiding this comment.
Thanks for following up. I investigated this specifically because the new expected image also looked as though nearest-neighbor sampling might have stopped working.
It has not. The changed output is caused by correcting the mapping of this fixture’s non-parallelogram image quad.
In calculated tile coordinates, the fixture’s corners are:
TL (5320, 7456)
TR (7240, 4232)
BR (9600, 5568)
BL (7904, 8904)
They have a parallelogram closure error of (-224, -112), so this fixture receives a genuine non-identity projective transform:
[-1.30458979876778e-5, 2.07071621334893e-5, 1]
I compared upstream main at 8ee0ba36a with this branch at 65a93ecb4, using Node 26.7.0 from .nvmrc, headless Chrome, and SwiftShader. The upstream fixture passes its upstream expected image, and the unchanged fixture on this branch passes the updated expected image.
The sampling checks were:
- On this branch, switching only
raster-resamplingfromnearesttolinearchanges 2,972 of 4,096 pixels exactly (72.5586%). If nearest were being ignored, this toggle would not change the output. - Replacing normal sampling with an exact level-0
texelFetchat the projectively divided UV produces a byte-for-byte identical image. A deliberately incorrect+1-texel control changes 84.9% of pixels, confirming that the control can detect the wrong texel. - An independent texel-cell model fits the branch render under projective nearest sampling 40–80× better than the incorrect affine pairing.
- GPU derivative instrumentation reports LOD
-2.039on every fragment (rho = 0.243), so all fragments are magnified. The texture minification filter and itsLINEARfallback are never used.
The image looks smoother because the corrected homography samples a different, flatter portion of the source photograph—not because the filtering became linear. At the viewport centre, the sampled source position moves from approximately (518.55, 231.40) to (498.17, 242.02), a displacement of about 23 source pixels.
Two controls isolate this further:
- Forcing the transform to identity reproduces the upstream render byte-for-byte.
- Closing the coordinates into an exact parallelogram makes upstream and this branch byte-for-byte identical.
The expected-image update is therefore required: on the same renderer, the upstream and corrected outputs differ by 267 significant pixels, while this 64×64 fixture allows only 1.
One separate issue surfaced: this old fixture is not a strong resampling regression test. On current main, changing it from nearest to linear still passes its checked-in expected image at the suite threshold, despite many small exact differences. That weakness predates this PR and should be addressed separately with a high-contrast, exact-parallelogram fixture.
So the conclusion is that this is intended projective behavior, not a nearest-neighbor regression, and no production shader or filtering fix is needed for this expected-image change.
However, one additional test-scope issue surfaced during this validation.
The new projection/globe/image-projective fixture does not currently discriminate the projective correction. With the correction completely disabled, it still passes: 4,763 of 262,144 pixels differ exactly, the maximum channel difference is 1, and 0 pixels are significant at the render-suite threshold, against a 65-pixel allowance.
This occurs because globe raster geometry is subdivided at least 32×32. The interior mesh vertices use bilinearly generated positions with linear texture coordinates, so applying the quad homography only at those vertices is effectively inert. This differs from the single-quad Mercator path, where the correction reproduces the inverse homography exactly.
I suggest retaining the fixture as useful globe compatibility coverage, but renaming it in this PR to projection/globe/image-non-parallelogram and changing its description to:
Tests rendering of a non-parallelogram image source on globe projection.
The expected image can be moved unchanged. Duplicating the fixture would not add independent coverage because neither copy would detect the projective correction.
Making subdivided globe rendering genuinely projective requires defining how the homography should interact with the curved mesh and should be handled as a separate design ticket. It does not block this PR or affect the conclusion that nearest-neighbor sampling remains active.
There was a problem hiding this comment.
Thanks for the research. I think it would be good to move the center of this test to a different place that will show the feature - e.g. show a pixelated image, which is what the resampling feature is.
Feel free to do any other renames you think will be useful, or change the description inside the style.json to improve the test description.
Thanks!!
If you prefer to do it in another PR so this can be merged, that's fine too, just let me know...
There was a problem hiding this comment.
Implemented in d415f0efe.
I moved the fixture center to:
[-122.5152017, 37.562688978]This keeps the same image, zoom, and non-parallelogram coordinates, but places the viewport over the high-contrast rock/surf region. The nearest render is now visibly pixelated, while the linear render is visibly smoothed.
The fixture now meaningfully enforces both behaviors:
- Nearest versus linear changes 3,902 of 4,096 pixels exactly, with 6 significant pixels against the test's 1-pixel allowance.
- With projective correction disabled, 1,757 pixels are significant, so the fixture remains strongly sensitive to projective mapping.
- Repeated renders of both resampling modes were byte-for-byte stable.
I regenerated the expected image and added a description explaining that this tests nearest-neighbor resampling on a projectively mapped image source.
I also renamed the new globe fixture from image-projective to image-non-parallelogram, since it provides globe compatibility coverage but does not independently discriminate the projective correction. Its expected image was moved unchanged.
The focused render suite passes. No production-code change was required.
The raster shader needs a mapping from rendered quad position In homogeneous form: The CPU code computes the complete denominator row of that inverse homography: Those three coefficients are homogeneous: multiplying all of them by the same nonzero scale does not change Before uploading the transform, The vertex shader passes The derivation is documented in the |
Use homogeneous texture coordinates for non-parallelogram image, canvas, and video source quads across Mercator, globe, and terrain. Preserve valid zero-constant transforms, fall back for singular or zero-crossing denominators, and add unit and render coverage. Fixes maplibre#7886.
04fe5fa to
91e5ffb
Compare
|
Superseded by #7903 with a clean linear commit history and neutral branch naming. |
|
We are squash-merging anyway so git history isn't important in the branch. |
|
The fresh CI run completed with one unrelated Windows render timeout in |
|
I've added my comments, once addressed this could be merged. Thanks! |
Reuse the raster program's perspective type and identity coefficients, restore focused draw-call formatting, and cover the initial ImageSource load path. Use gl-matrix primitives with double-precision arrays and reject cancellation-degenerate homogeneous denominators. Merge current main and regenerate the bundle-size baseline.
…projective-raster # Conflicts: # CHANGELOG.md # test/build/bundle_size.json
|
@HarelM Done. |
|
2 open comments left, otherwise can be merged. Thanks! |
d28fcd8 to
d415f0e
Compare
|
@i4innovationnet can you take a look at the videos in the following PR: It's a tool I'm helping maintain that was waiting for this fix to allow changing an image corners. Do you think there might be an edge case here that needs to be fixed? |
|
CC: @igalgh |
I don’t believe the native video shows a bug in #7887. The effect is continuous in the video—even around the apparent flip—and the grid lines remain straight without the old affine diagonal seam. The custom layer behaves differently because it uses a subdivided bilinear mesh. That spreads the image more evenly across the quad, which is often preferable for free-form editing, but it is a different warp model rather than a correction to the projective mapping. There is one real boundary caveat: at exact collinearity, or once the quad becomes concave, no valid invertible homography exists. The native implementation deliberately falls back to affine sampling there, which can cause an abrupt change and restore the diagonal seam. I agree that focused regression coverage would be useful. I suggest testing:
For the editing tool, the best protection is to keep corners away from that boundary using a scale-relative distance or area constraint rather than a fixed two-screen-pixel margin. So my conclusion is: no projective-math fix appears necessary for valid convex quads, but the near-degenerate and fallback behavior should be explicitly tested. Matching the custom layer would require a separate bilinear warp mode with mesh subdivision. So this is a separate, pre-existing near-degenerate-quad edge case that needs its own fix. |
|
Not sure I fully understand everything you wrote, but I think the near triangle shape feels like a bug worth fixing here, wouldn't you agree? |
|
@HarelM Yes, I agree that the near triangle case feels like a bug and is worth fixing. The distinction I’m making is that it is not a regression in #7887. Before it, the near triangular and other non-parallelogram quads were already rendered incorrectly as two affine triangles with a diagonal seam. #7887 fixes the projective mapping for valid quads, but the behavior as a quad approaches degeneracy remains an existing edge case. So I suggest addressing it in a separate follow-up. |
|
Ahh, no, definitely not a regression due to this PR, just another case worth fixing which I hoped you could take a look at since it seems you understand this code area very well. |
|
I've opened the following PR: Any feedback is welcomed. |
Launch Checklist
CHANGELOG.mdunder the## mainsection.Summary
Fixes projective rendering for image source quads whose four coordinates do not form a parallelogram.
The raster shader now carries homogeneous texture coordinates for image, canvas, and video source quads and divides by the projective denominator in the fragment shader before sampling. Parallelogram quads and regular tiled raster sources keep affine behavior through an identity perspective transform.
ImageSourcecomputes the complete inverse-homography denominatorw = ax + by + c, rejects singular, non-finite, and zero-crossing transforms, and normalizes the homogeneous coefficients without assuming thatcis nonzero. This preserves valid convex trapezoids while failing closed for degenerate, concave, and self-intersecting coordinates.The texture correction remains separate from clip-space projection:
projectTileownsgl_Position.wfor Mercator, globe, and terrain paths, while this change performs the texture divide explicitly in the fragment shader.Related Issue
Fixes #7886.
Visuals
The before screenshot and original reproduction are in #7886.
Render fixtures cover the corrected non-parallelogram case, a valid trapezoid whose homogeneous denominator has no constant term, and the projection modes discussed during review:
test/integration/render/tests/image/projective/style.jsontest/integration/render/tests/image/projective-trapezoid/style.jsontest/integration/render/tests/projection/globe/image-non-parallelogram/style.jsontest/integration/render/tests/terrain/image-projective/style.jsonThe projective fixtures use
projective-grid.pngand include semi-transparent vector outlines so the expected corner geometry remains visible.Reviewer Follow-Up
ImageSource; regular tiled raster sources explicitly pass identity coefficients._finishLoading()tosetCoordinates(); added an initial-load regression test instead of duplicating the calculation inload().gl-matrixprimitives backed by plain JavaScript arrays to preserve double precision.ImageSourcestate tests.local://image/0.png, recentered it over a high-contrast region so nearest and linear exceed the render-test threshold, and regenerated its expected output.image-non-parallelogramso its name reflects compatibility coverage rather than implying that it discriminates the projective correction.Number.EPSILON-scaled singularity checks.c = 0and rejected denominators that reach or cross zero inside a quad.mainand regenerated the combined bundle-size baseline.Validation
Local validation on Node
24.11.1, after merging currentmain:npm cinpm run lint -- --max-warnings 0npm run lint-cssnpm run generate-typingsnpm run typechecknpm run generate-docsnpm run build-distnpm run test-unit-ci— 198 files, 2,917 tests passednpm run test-build-ci— 8 files, 699 tests passednpm run test-integration-ci— 4 files, 176 tests passednpm run test-render -- -t 'tests/(image/(projective|projective-trapezoid|default|raster-resampling)|projection/globe/image-non-parallelogram|terrain/image-projective|canvas/default|video/default|raster-resampling/default)'— 9 affected render tests passedgit diff --checkLatest focused validation on Node
26.7.0from.nvmrc:npm run test-render -- -t 'tests/(image/(raster-resampling|projective|projective-trapezoid)|projection/globe/image-non-parallelogram|terrain/image-projective)'— 5 passedimage/raster-resampling— failed as intended with 6 significant pixels against 1 allowedgit diff --checkThe prior Windows integration failure was a Puppeteer navigation timeout in an untouched query-test setup path. It did not reproduce locally; the full integration suite passed twice after the production build prerequisite.