Conversation
✅ Deploy Preview for afmg ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
Pull request overview
Refactors label handling to move state and burg label information into a data layer (pack.labels) while keeping rendering in SVG renderers, aiming to make labels more data-driven and reusable across the app.
Changes:
- Added a
Labelsmodule to generate/store/update state and burg label data inpack.labels - Extracted state-label raycast logic into a reusable
src/utils/label-raycast.tsutility - Updated state/burg label renderers and map generation to use/populate the new label data
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/utils/label-raycast.ts | New shared raycast + scoring utilities for state label path selection |
| src/types/PackedGraph.ts | Extends PackedGraph with labels: LabelData[] |
| src/renderers/draw-state-labels.ts | Renders state labels from Labels data and updates label data (text/font size) during fitting |
| src/renderers/draw-burg-labels.ts | Renders burg labels from Labels data and syncs offsets back to label data |
| src/modules/labels.ts | New global Labels module for label CRUD + generation from states/burgs |
| src/modules/index.ts | Ensures Labels module is loaded/registered |
| public/main.js | Calls Labels.generate() during map generation |
| if (labels.length === 0) return 0; | ||
|
|
||
| const existingIds = labels.map((l) => l.i).sort((a, b) => a - b); | ||
| for (let id = 0; id < existingIds[existingIds.length - 1]; id++) { | ||
| if (!existingIds.includes(id)) return id; | ||
| } | ||
| return existingIds[existingIds.length - 1] + 1; |
There was a problem hiding this comment.
getNextId recomputes and sorts all IDs on every call and then uses includes inside a loop, which becomes quadratic as labels grow (burg labels can be numerous). Consider tracking the next id incrementally and/or using a Set to find gaps, or maintaining a monotonically increasing counter when gaps are not required.
| if (labels.length === 0) return 0; | |
| const existingIds = labels.map((l) => l.i).sort((a, b) => a - b); | |
| for (let id = 0; id < existingIds[existingIds.length - 1]; id++) { | |
| if (!existingIds.includes(id)) return id; | |
| } | |
| return existingIds[existingIds.length - 1] + 1; | |
| if (!labels || labels.length === 0) return 0; | |
| const existingIds = new Set<number>(); | |
| let maxId = -1; | |
| for (const label of labels) { | |
| const id = label.i; | |
| existingIds.add(id); | |
| if (id > maxId) maxId = id; | |
| } | |
| for (let id = 0; id <= maxId; id++) { | |
| if (!existingIds.has(id)) return id; | |
| } | |
| return maxId + 1; |
There was a problem hiding this comment.
@StempunkDev TODO: cache ids more intelligent if needed for performance
|
@SheepFromHeaven do you intend on migrating any of the editor in the near future? Since I would need to edit the burgs-editor.js for feature completeness. I hope I can safe both of us some merge conflicts |
|
No worries. I will then leave it out for now. There is still enough other things. Incl. Fixing bugs 😂 |
…version to 1.113.0
…l rendering logic
…in editLabel function
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
Update: Mostly done. Custom Labels Migration from Older Maps still need to be tested.
|
|
Technically done. @Azgaar was the behavior of labels that they reset each time they are drawn/loaded or where there positions saved? Since when the labels are redrawn they reset due to group overriding the changes. I think this is both true for State and Burg Labels. Is that acceptable? |
If we can not rerender them completely on minor changes, we should not reredner them. |
3 participants
Description
This PR aims to split the view and data of the state and burg labels. What will be missing are the province labels. The goal is to get labels working as data as much as possible for states and burgs.
Type of change
Versioning