Fixes stale label state on unbind

[mzschwartz5]

Description

In #12905, I updated Label to not clamp its individual glyph billboards, but to instead use the parent's position. This was a performance optimization, since clamping is expensive and not strictly necessary on individual label glyphs.

This change introduced a regression, as (it turns out) when labels are deleted, the underlying glyph billboards are not deleted, but stored (in _spareBillboards on the LabelCollection) and reused when new labels are created. Certain fields are not reset before being reused (namely, in this case, _clampedPosition), which caused some misplaced labels in workflows that delete and recreate clamped labels.

The fix simply resets the field before storing the label's glyph billboard in _spareBillboards

Issue number and link

#12949

Testing plan

Zoom in, then out (perhaps a few times, find the right zoom level) on this sandcastle. Note how certain labels start to overlap.

Try the same on this sandcastle (deployed version from this PR). It should no longer occur.

Author checklist

• I have submitted a Contributor License Agreement
• I have added my name to CONTRIBUTORS.md
• I have updated CHANGES.md with a short summary of my change
• I have added or updated unit tests to ensure consistent code coverage
• I have updated the inline documentation, and included code examples where relevant
• I have performed a self-review of my code

[github-actions]

Thank you for the pull request, @mzschwartz5!

✅ We can confirm we have a CLA on file for you.

[donmccurdy]

Tested and works well locally!

One thought about the location of billboard-reset logic, but no need to address that, it is a bit outside the PR's scope. I'll merge later today, if that's OK.

[donmccurdy]

Optional and non-blocking thought, especially since it predates this PR, but — I wonder if the Billboard class would benefit from having a public .reset(), .remove(), or .unbind() method? Seems like we're having to manage Billboard's private members here in LabelCollection, maybe that could be internal to Billboard.

[mzschwartz5]

I totally agree, I will admit I was feeling kind of lazy here 😅 but the thought crossed my mind.

I'm on another bug fix at the moment, but will see if I can get to that today. Otherwise, if we end up merging, I'll open an issue (could be an onboarding issue even).

[javagl]

If I had to summarize my thoughts about accessing private properties from other classes in general, then that summary would be a (somewhat confused) "Don't 🤷‍♂️".

One could make some cases about Billboard and BillboardCollection being kinds of "(C++) friend" classes, or certain properties being considered as "(Java) 'package-private'". But some classes seem to be friend's friend's friend's friend's friends.

(Of course, one could bring that up in any PR, so ... let's consider this to be tracked, e.g. in the issue around this comment, and hope that it will be addressed explicitly at some point).

[donmccurdy]

Merged! No particular pressure on extra steps — refactoring could turn into a bigger scope than this fix, and just opening an issue would be more than enough IMO.

TypeScript has a distinction between private and @internal tags, similar to Java package-private. That can occasionally be useful, but doesn't exist in JSDoc to my knowledge. In this case ... the original choice of private seems reasonable, I guess the private concerns just escaped a bit over time. :)