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

Port symbol-placement: line-center to native #12337

Merged
merged 4 commits into from
Jul 23, 2018
Merged

Conversation

ChrisLoer
Copy link
Contributor

Fixes #12300.

Removes unused "maxCameraDistance" shader uniforms (these have been obsolete since the switch to viewport collision detection and render-time line-label layout).

Also changes collision detection behavior for line labels with width less than 1/2 of a collision circle (previously, such labels wouldn't show).

Using an unmerged pin of GL JS for tests.

text-max-angle/line-center is currently failing because one extra label is showing up -- I haven't dug into why yet.

/cc @ansis @nickidlugash

@ChrisLoer ChrisLoer added the ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold label Jul 6, 2018
@ChrisLoer ChrisLoer force-pushed the symbol-line-center branch from 2efbc06 to ad686a0 Compare July 10, 2018 18:40
@ChrisLoer ChrisLoer removed the ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold label Jul 10, 2018
@ChrisLoer ChrisLoer requested a review from ansis July 10, 2018 18:41
@ChrisLoer
Copy link
Contributor Author

This is now good to go, except that it depends on two separate gl-js PRs for updated tests:

@ChrisLoer
Copy link
Contributor Author

@ansis do you know if you'll get a chance to review this soon? It's not urgent, but if it'll take more than a day, I should push some test ignores to GL JS (currently blocking #12415 cc @jfirebaugh)

@ChrisLoer ChrisLoer force-pushed the symbol-line-center branch from 5b80ea0 to c671c82 Compare July 20, 2018 20:04
- Remove unused/vestigial 'maxCameraDistance'
- Create a single collision circle for line labels that are less than half the width of a collision circle
@ChrisLoer ChrisLoer force-pushed the symbol-line-center branch from c671c82 to 2770b44 Compare July 20, 2018 20:06
Copy link
Contributor

@ansis ansis left a comment

Choose a reason for hiding this comment

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

This looks good to me! Sorry for the delay.

The redundant avoidEdges code could be removed.

// CJL: I'm not sure why SymbolPlacementType::Line -> avoidEdges = false. It seems redundant since
// getAnchors will already avoid generating anchors outside the tile bounds.
// However, SymbolPlacementType::LineCenter allows anchors outside tile boundaries, so its behavior
// here should match SymbolPlacement::Point
Copy link
Contributor

Choose a reason for hiding this comment

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

I think in it's current form this is completely redundant, but I think there is a potential static tile rendering bug this reminds me of:

While the anchors will always be within the tile bounds, the label itself could still cross tile boundaries. This is fine for regular gl maps, but for static tile rendering these labels would cause problems. Since the position of line labels is based on tile clipping, their position is unpredictable and would likely be visibly clipped in static tile maps.

This also applies to line center labels.

But we don't have any support for avoid edges in the current collision code, right? Is the current version of symbol placement in production for static tile maps?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yeah, is this just a loose end we left behind? I was thinking this was handled by including the tile buffers in collision detection for static tiles, but you're right that doesn't work for line labels. It sounds to me like we probably need to re-implement some kind of avoidEdges support for line labels. Filed as #12461. Since we'll probably want to re-implement it, we might as well leave the rump of the avoidEdges logic in place for now.

@ChrisLoer ChrisLoer merged commit da5966b into master Jul 23, 2018
@ChrisLoer ChrisLoer deleted the symbol-line-center branch July 23, 2018 20:33
@friedbunny friedbunny added Core The cross-platform C++ core, aka mbgl rendering labels Aug 13, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Core The cross-platform C++ core, aka mbgl rendering
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants