Skip to content

Conversation

@gabeboning
Copy link
Contributor

@gabeboning gabeboning commented Dec 30, 2025

Background

When auto packing is enabled, the icon manager must reserve space for the width/height declared by the icon definition, even before icon is loaded. After an icon loads, it is resized and centered in that reserved space/box. #7651 fixed a bug where the icon image itself would be stretched to completely fill the box, distorting the icon if it had an aspect ratio which differed from that of the box. However, for the icon mapping that the icon manager returns, that PR only updated the width/height fields, and not the x/y. So when retrieving a mapping, an icon frame would be returned with the correct size, but not centered where the actual icon is, resulting in icons not displaying properly.

Change List

  • Update icon definition x/y after resizing icon

@gabeboning
Copy link
Contributor Author

@Pessimistress could you review this, since I think you'll have the most SA based on #7651?

@coveralls
Copy link

coveralls commented Jan 7, 2026

Coverage Status

coverage: 91.115% (+0.001%) from 91.114%
when pulling 5b848a8 on gabeboning:fix-icon-frame
into 7f96305 on visgl:master.


private _onUpdate(): void {
this.setNeedsRedraw();
this.getAttributeManager()?.invalidate('getIcon');
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an expensive operation called on every icon image load. Pass a flag to onUpdate for whether icon frame is changed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done!

Copy link
Collaborator

@Pessimistress Pessimistress left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@felixpalmer felixpalmer merged commit 13ac1a8 into visgl:master Jan 16, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants