-
Notifications
You must be signed in to change notification settings - Fork 3.7k
Fixes stale label state on unbind #13086
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
Thank you for the pull request, @mzschwartz5! ✅ We can confirm we have a CLA on file for you. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
| const billboard = glyph.billboard; | ||
| if (defined(billboard)) { | ||
| billboard.show = false; | ||
| billboard._clampedPosition = undefined; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
Description
In #12905, I updated
Labelto 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
_spareBillboardson theLabelCollection) 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
_spareBillboardsIssue 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
CONTRIBUTORS.mdCHANGES.mdwith a short summary of my change