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

HARP-12581: Fix render order for labels #1980

Merged
merged 4 commits into from
Dec 1, 2020
Merged

HARP-12581: Fix render order for labels #1980

merged 4 commits into from
Dec 1, 2020

Conversation

atomicsulfate
Copy link
Collaborator

@atomicsulfate atomicsulfate commented Nov 19, 2020 •
edited
Loading

  • Set Render order from the technique in TextElement or PoiInfo.
  • Use DataSource.dataSourceRenderOrder to compose with Technique's renderOrder into a single bigint renderOrder.
  • Add unit and IBCT tests cases.

@atomicsulfate atomicsulfate force-pushed the HARP-12581_Labels branch 7 times, most recently from 81eb415 to 320dbec Compare November 24, 2020 16:24
@atomicsulfate atomicsulfate force-pushed the HARP-12581_Labels branch 2 times, most recently from 1962ade to 096d992 Compare November 25, 2020 14:05
@atomicsulfate atomicsulfate changed the title HARP-12581: Add dynamic markers example. HARP-12581: Fix render order for labels Nov 25, 2020
@atomicsulfate atomicsulfate force-pushed the HARP-12581_Labels branch 3 times, most recently from 5a41146 to a0aba71 Compare November 25, 2020 16:10
@codecov
Copy link

codecov bot commented Nov 25, 2020 •
edited
Loading

Codecov Report

Merging #1980 (e3de4b0) into master (b5525fe) will increase coverage by 0.04%.
The diff coverage is 92.30%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1980      +/-   ##
==========================================
+ Coverage   66.20%   66.24%   +0.04%     
==========================================
  Files         295      295              
  Lines       26246    26281      +35     
  Branches     5910     5919       +9     
==========================================
+ Hits        17376    17410      +34     
- Misses       8870     8871       +1
Impacted Files Coverage Δ
@here/harp-mapview/lib/poi/PoiManager.ts 12.85% <0.00%> (ø)
@here/harp-mapview/lib/text/TextElementBuilder.ts 98.93% <95.65%> (-1.07%) ⬇️
...e/harp-mapview/lib/geometry/TileGeometryCreator.ts 70.53% <100.00%> (ø)
@here/harp-mapview/lib/text/TextElement.ts 93.24% <100.00%> (ø)
...atasource-protocol/lib/operators/ArrayOperators.ts 93.93% <0.00%> (-2.22%) ⬇️
@here/harp-text-canvas/lib/TextCanvas.ts 67.23% <0.00%> (+0.42%) ⬆️
@here/harp-utils/lib/assert.ts 54.54% <0.00%> (+9.09%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b5525fe...e3de4b0. Read the comment docs.

@atomicsulfate atomicsulfate marked this pull request as ready for review November 25, 2020 16:17
FraukeF
FraukeF reviewed Nov 26, 2020
View reviewed changes
@here/harp-mapview/lib/text/TextElementBuilder.ts
private readonly m_styleCache: TileTextStyleCache
) {}
private readonly m_styleCache: TileTextStyleCache,
private readonly m_baseRenderOrder: number
Copy link
Contributor

Choose a reason for hiding this comment

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

I saw that later in the tests, you pass a lot of zeros for the baseRenderOrder, maybe you can default to 0 instead

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I rather pass the extra zeros explicitely in the test. Having a default value has the risk that someone forgets to pass the datasourceOrder.

@here/harp-mapview/lib/poi/BoxBuffer.ts Outdated Show resolved Hide resolved
@here/harp-mapview/lib/text/TextElement.ts Outdated Show resolved Hide resolved
@here/harp-vectortile-datasource/test/resources/geoJsonData.ts Outdated Show resolved Hide resolved
@atomicsulfate
HARP-12581: Change bigint to number to keep supporting render orders …
e3de4b0 
…with decimals.

 - finalRenderOrder = dataSourceOrder * 10^7 + renderOrder
 - Warn if render order values are too big
@FraukeF FraukeF merged commit 0f31957 into master Dec 1, 2020
@FraukeF FraukeF deleted the HARP-12581_Labels branch December 1, 2020 07:22
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@FraukeF FraukeF FraukeF approved these changes

@robertoraggi robertoraggi Awaiting requested review from robertoraggi

Assignees
No one assigned
Labels
topic:HARP-12581_Labels
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@atomicsulfate @robertoraggi @FraukeF