update glyph placement to verify all glyphs are valid

[ragnarok56]

Resolves #1224

Addresses an issue with glyphs where first/last glyphs are assumed to be sufficient to place all remaining glyphs using placeGlyphAlongLine which can return a null value and throws an exception later in placeGlyphsAlongLine.

Launch Checklist

• Confirm your changes do not include backports from Mapbox projects (unless with compliant license) - if you are not sure about this, please ask!
• Briefly describe the changes in this PR.
• Link to related issues.
• Include before/after visuals or gifs if this PR includes visual changes.
• Write tests for all new functionality.
• Document any changes to public APIs.
• Post benchmark scores.
• Add an entry to CHANGELOG.md under the ## main section.

[codecov]

Codecov Report

Attention: Patch coverage is 66.66667% with 2 lines in your changes missing coverage. Please review.

Project coverage is 91.81%. Comparing base (30ab703) to head (14b9de2).
Report is 4 commits behind head on main.

Files with missing lines	Patch %	Lines
src/symbol/projection.ts	66.66%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #5118   +/-   ##
=======================================
  Coverage   91.81%   91.81%           
=======================================
  Files         279      279           
  Lines       38340    38344    +4     
  Branches     6698     6699    +1     
=======================================
+ Hits        35200    35204    +4     
  Misses       3007     3007           
  Partials      133      133           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[HarelM]

Thanks for taking the time to solve this!
It would be great if you could create a test for this.
Hopefully a render test to showcase the issue and the solution.
Let me know if you need help with wiring the test.

[ragnarok56]

Trying to come up with a render test that can recreate it, but having some issues identifying the exact scenario that causes it to fail. I don't have access to the environment this is was occurring in right now. I'm also not certain if it 100% happens on just rendering or if it was being triggered from zooming in/out of the area and maybe that is part of the calculus involved, if so then a render test may not be sufficient by itself?

Also not at all familiar with how placeGlyphAlongLine works and how to trigger the error case where one of the symbols fails to be placed along a line after the first/last check is done.

[HarelM]

If you can't create a test to reproduce it, it will be impossible in the future to make sure this won't break again...
Also it will make testing that this actually fixes the issue harder...

[bgeihsgt]

Commenting on this as our users hit this from time to time and would love if this could get merged. Were you guys able to work out how to test this?

[HarelM]

I haven't seen any updates regarding a test to showcase this issue and the relevant fix...

[ragnarok56]

background

For more context on where I've run into this issue, we're running maplibre inside of deck.gl as a static basemap layer. We use osm vector tiles and osm bright style. The error is reliably reproduced only when transitioning between zoom levels and certain features with multiline text labels. We mitigated the issue by just removing the newline, so this doesn't actually occur for us (that we're aware of) anymore, see my initial comment on #1224. When we load the same style and map data using tileserver-gl and zoom in/out over the same features that cause the problem it works fine.

My theory is that there is some condition where deck.gl managed zoom transitions with maplibre-gl end up triggering the condition exhibited by the issue, but maplibre-gl itself doesn't cause the scenario (at least reliably)

what this pr is addressing

I was unable to create an integration render test that could replicate the error case. That isn't to say that there isn't an issue, there is still a logic flaw in the code that this PR is addressing by adding a guard clause that mimics other scenarios

placeGlyphAlongLine can return null
https://github.com/maplibre/maplibre-gl-js/blob/main/src/symbol/projection.ts#L839

        // offset does not fit on the projected line
        if (currentIndex < lineStartIndex || currentIndex >= lineEndIndex)
            return null;

every other invocation of this function has a guard check to see if it returned null, and ends up returning a {notEnoughRoom: true} result to short circuit the handling... except for:
https://github.com/maplibre/maplibre-gl-js/blob/main/src/symbol/projection.ts#L469

            placedGlyphs.push(placeGlyphAlongLine(fontScale * glyphOffsetArray.getoffsetX(glyphIndex), lineOffsetX, lineOffsetY, flip, symbol.segment,
                lineStartIndex, lineEndIndex, projectionContext, rotateToLine));

where it ends up throwing an exception later where glyph ends up being null

for (const glyph of placedGlyphs) {
        addDynamicAttributes(dynamicLayoutVertexArray, glyph.point, glyph.angle);
    }

Perhaps this is where a unit test is needed instead of an integration render test? I don't understand enough of the internals to craft the test data that can force placeGlyphAlongLine to enter the failure case where it returns null from the starting point of a render test.

[HarelM]

Sure, if a render/integration test is something we have s hard time creating, a unit test will be enough, but a test is still needed to prevent this from happening in the future.