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

place symbol layers in correct order #3543

Merged
merged 1 commit into from
Jan 15, 2016
Merged

Conversation

ansis
Copy link
Contributor

@ansis ansis commented Jan 14, 2016

Previously, the placement of symbol layers depended on the order in which the glyphs they need were loaded. The placement order should be based on the style.

To fix this, symbol layers are placed in placementPending after they are parsed. After all layers are parsed, symbol layers (stored in placementPending) are placed and returned.

@kkaefer can you review this?

// We cannot parse this bucket yet. Instead, we're saving it for later.
pending.emplace_back(layer->as<SymbolLayer>(), std::move(bucket));
} else {
placementPending.emplace(layer->bucketName(), std::move(bucket));
Copy link
Contributor

Choose a reason for hiding this comment

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

This will not overwrite existing data that has the same key in placementPending. Is this what you want?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since label names are unique, there should never be anything to overwrite. Also, this is the same as this

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 just checking

@ansis
Copy link
Contributor Author

ansis commented Jan 14, 2016

@jfirebaugh it would be great to get a third set of 👀 on this if you have time

@jfirebaugh
Copy link
Contributor

Will do.

partialParse = false;
placementConfig = config;
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of storing a copy of placementConfig, would it be better to pass the correct value to TileWorker::parsePendingLayers, and then pass it through as a parameter to placeLayers everywhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, that would be cleaner and it could avoid an extra redoPlacement in some cases. changed in 716b01c

@ansis
Copy link
Contributor Author

ansis commented Jan 14, 2016

is this ready for squashing and merging?

@jfirebaugh
Copy link
Contributor

LGTM

Previously, the placement of symbols depended on the order in which the
glyphs they need were loaded. The placement order should be based on the
style.

To fix this, symbol layers are placed in `placementPending` after they
are parsed. After all layers are parsed, symbol layers (stored in
`placementPending`) are placed and returned.
@ansis ansis force-pushed the strict-symbol-placement-order branch from 6016ea0 to e44db93 Compare January 15, 2016 01:51
@ansis ansis merged commit e44db93 into master Jan 15, 2016
@ansis ansis removed the in progress label Jan 15, 2016
@ansis ansis deleted the strict-symbol-placement-order branch February 3, 2016 01:31
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants