Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

[core] fix collisionBox alignment when Icon/text translation is enabled #15467

Merged
merged 9 commits into from
Aug 30, 2019
Merged

[core] fix collisionBox alignment when Icon/text translation is enabled #15467

merged 9 commits into from
Aug 30, 2019

Conversation

zmiao
Copy link
Contributor

@zmiao zmiao commented Aug 23, 2019 •
edited
Loading

Fix partially of #13526
Render test: mapbox/mapbox-gl-js#8659

@zmiao zmiao changed the title [Core] fix collisionBox alignment when Icon/text translation is enabled [core] fix collisionBox alignment when Icon/text translation is enabled Aug 23, 2019
@zmiao zmiao self-assigned this Aug 23, 2019
@chloekraw chloekraw added the needs changelog Indicates PR needs a changelog entry prior to merging. label Aug 26, 2019
@zmiao zmiao requested a review from 1ec5 as a code owner August 28, 2019 10:41
@zmiao zmiao requested a review from a team August 28, 2019 10:41
pozdnyakov
pozdnyakov approved these changes Aug 29, 2019
View reviewed changes
Copy link
Contributor

@pozdnyakov pozdnyakov left a comment

Choose a reason for hiding this comment

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

Looks nice! My only concern is memory usage, @zmiao do you have some estimations of the memory consumption impact caused by the change?

src/mbgl/layout/symbol_layout.cpp Outdated Show resolved Hide resolved
src/mbgl/layout/symbol_layout.cpp Outdated Show resolved Hide resolved
src/mbgl/layout/symbol_layout.cpp Outdated Show resolved Hide resolved
src/mbgl/layout/symbol_layout.cpp Outdated Show resolved Hide resolved
@astojilj
Copy link
Contributor

Looks nice! My only concern is memory usage, @zmiao do you have some estimations of the memory consumption impact caused by the change?

@pozdnyakov , @zmiao
I didn't review the patch yet... what could cause increased memory consumption?

@pozdnyakov
Copy link
Contributor

what could cause increased memory consumption?

The icon collision data might require more memory

alexshalamov
alexshalamov approved these changes Aug 29, 2019
View reviewed changes
Copy link
Contributor

@alexshalamov alexshalamov left a comment

Choose a reason for hiding this comment

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

lgtm, for rendering fix, +1 for comments from @pozdnyakov

One thing is unclear, should we move collision feature when icon / text is translated? If so, #13526 is only partially fixed.

@zmiao
Copy link
Contributor Author

zmiao commented Aug 29, 2019

@pozdnyakov @astojilj the number of the vertexes remains the same, only change is the container is split as two which hold text and icon separately. So no extra memory is added.

@pozdnyakov
Copy link
Contributor

@pozdnyakov @astojilj the number of the vertexes remains the same, only change is the container is split as two which hold text and icon separately. So no extra memory is added.

I guess it's just + sizeof(CollisionBoxBuffer) per bucket, think its fine. Thanks!

@zmiao zmiao merged commit 6ef2710 into master Aug 30, 2019
@zmiao zmiao mentioned this pull request Aug 30, 2019
Open
@zmiao zmiao deleted the icon/text_translation branch August 30, 2019 11:03
@friedbunny friedbunny added this to the release-ristretto milestone Sep 19, 2019
@friedbunny friedbunny added Core The cross-platform C++ core, aka mbgl and removed needs changelog Indicates PR needs a changelog entry prior to merging. labels Sep 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@pozdnyakov pozdnyakov pozdnyakov approved these changes

@alexshalamov alexshalamov alexshalamov approved these changes

@kkaefer kkaefer Awaiting requested review from kkaefer

@1ec5 1ec5 Awaiting requested review from 1ec5

Assignees

@zmiao zmiao

Labels
Core The cross-platform C++ core, aka mbgl rendering
Projects
None yet
Milestone
release-ristretto
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@zmiao @astojilj @pozdnyakov @alexshalamov @friedbunny @chloekraw