Repository navigation
fix(plugins): a panel tree holds 20,000 nodes and says when it cuts (#1546) - #1738
elpidioassinaturas wants to merge 2 commits into
Conversation
…pengeos#1546) A plugin panel tree kept at most 5,000 nodes: too few for Brazil's municipalities with the IBGE levels above them (6,298), or Spain's, Italy's and Germany's municipalities alone. - MAX_TREE_NODES is 20,000. - Nodes past the limit are counted where the tree stops (`past` in tree_nodes), apart from those left out for other reasons. The answer's warning names the tree, how many nodes it was given and the limit; the item state keeps it (`cut`, also in `plugin.panel`) and the panel shows a note under the tree. - Each label is folded once, when the node is made (TreeNode::new), and a search compares against it (TreeNode::holds_words) instead of folding every label at every frame. `fold` moves to the plugins crate as TreeNode::fold, with the unicode-normalization dependency. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: true
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| } | ||
| if self.nodes > 0 { | ||
| parts.push(format!( | ||
| "{} option(s) or tree node(s) left out (no id, one repeated or too long, too deep, or past the limit)", |
There was a problem hiding this comment.
Nit (wording, low severity): this generic count no longer includes nodes past the limit. For trees, apply subtracts past from it, and nodes past the limit are reported apart by the cut notice above. For dropdowns, choice_options_reported never reaches the limit break, so it can't count one either. The text still lists "past the limit" as a reason, which is now misleading. Suggested wording:
| "{} option(s) or tree node(s) left out (no id, one repeated or too long, too deep, or past the limit)", | |
| "{} option(s) or tree node(s) left out (no id, one repeated or too long, or too deep)", |
Existing tests only check the prefix 3 option(s) or tree node(s) left out, so this change doesn't affect them.
There was a problem hiding this comment.
Right for trees, fixed in e57a4ea, with a different wording: a dropdown can still reach this count. An update's options past MAX_CHOICES are counted here, since apply adds options.len() minus those kept, and take(MAX_CHOICES) in choice_options_reported drops them before the loop. So the reason now reads "past a dropdown's limit" rather than being removed. a_tree_cut_at_its_limit_says_so now also updates a dropdown with MAX_CHOICES + 1 options and checks that the count says 1, that the notice names the dropdown's limit, and that no cut is set.
Code reviewBugsNone found. I checked the tree-cut accounting ( SecurityNone found. The cut notice and panel note only print sanitised ids and numbers. PerformanceNone found. Label folding now happens when a plugin answer is applied ( Quality
AGENTS.mdNo violations found.
Prior review threads: none existed on this PR, so I left no earlier concerns out. Change sizeCompared: the whole PR against main (merge base 7cc9bb6 to PR head 7434545).
|
📖 Docs preview
GitHub Pages takes a minute or two to publish a new commit. |
…t a tree's (opengeos#1546) A tree's cut is said apart, so "past the limit" in the generic count now reads "past a dropdown's limit": the options of an update past MAX_CHOICES are still counted there. The test covers both. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| for (i, n) in v.iter().enumerate() { | ||
| if *left == 0 { | ||
| // The rest of this level, and below it; the levels above count their own rest. | ||
| *past = past.saturating_add(count_given(v.get(i..).unwrap_or_default(), depth)); |
There was a problem hiding this comment.
Low severity, a wording/semantics point rather than a loss: the cut is decided before a node is validated, so when the limit is hit and the rest of the list holds only nodes that would never be kept (no id, a repeated id, too deep), they are still counted in past and the tree is reported as cut. For example, 20,000 valid nodes followed by one node without an id gives "was given 20001 nodes: only the first 20000 are kept", and the panel note says a search may find what was left out, though nothing a search could find was lost.
The count is accurate as "given", so this may be intended. If you want the cut to mean "a valid node was lost", the cheap fix is to count only the remaining nodes that pass the id check, or to say "given" and "kept" without implying a loss. I'd leave the choice to you; there is no test for this case yet.
There was a problem hiding this comment.
Intended; no change. Both texts state counts, "given" and "kept", and both stay true in this case. They don't say a node a search could find was lost. The panel note reads "Only the first 20000 of the 20001 nodes the plugin gave are shown". Nodes past the limit are counted, not read. Telling which of them would have been kept would mean running the id, duplicate and depth checks on everything past 20,000, which is the work the limit bounds. Either way the plugin's fix is the same, since it sent more nodes than a tree holds. Those nodes are therefore never counted again among the ones left out for other reasons, so nothing is said twice.
Code reviewI found no blocking bugs. The tree-cut accounting holds up: Bugs
Security
Performance
Quality
AGENTS.md
Earlier concerns left alone: 1 thread (2 comments: the generic-count wording, answered by the author with a fix). Posted: 1 inline comment. Change sizeCompared: the whole PR against main (merge base 7cc9bb6 to PR head e57a4ea).
|
Closes #1546
A plugin panel tree (#1050, #1080) kept at most 5,000 nodes. That is too few for a country's administrative units: Brazil's 5,570 municipalities with the IBGE levels above them make 6,298 nodes, and Spain, Italy and Germany pass the limit with the municipalities alone. The BrasilData plugin, whose panel picks the work area from the IBGE boundaries, has had to send one region at a time, so a search for a municipality in a region not yet chosen finds nothing.
What changes
MAX_TREE_NODESis 20,000. That covers Brazil with its whole hierarchy, and Spain (~8,100), Italy (~7,900) and Germany (~10,800).<plugin>: tree catalogue was given 20001 nodes: only the first 20000 are kept, the most a tree holds. The panel shows a note under the tree ("Only the first 20000 of the 20001 nodes the plugin gave are shown: a tree holds at most 20000."), also when a search finds nothing, since what it looks for may be among those left out.plugin.panelanswers the tree's item with"cut": <nodes given>.tree_nodeskeeps the folded label (lower case, NFD, marks dropped) inTreeNodewhen the nodes arrive, and a search compares against it (TreeNode::holds_words). Before,tree_rowsfolded every node's label at every frame while a search was typed.What #1667 already covered
#1667 (Plugins: report lossy panel updates) already counted nodes left out of an answer in one warning:
N option(s) or tree node(s) left out (no id, one repeated or too long, too deep, or past the limit). That stays for the other reasons. What it didn't say is which tree was cut at the limit, from how many nodes, and nothing showed in the panel. Nodes past the limit are now counted apart from that generic count, so a cut isn't said twice.Choices
tree_nodesadds the rest of a level, with what it holds, to apastcounter when it stops at the limit; the levels above count their own rest. Nodes left out for other reasons (no id, repeated, too deep) stay inDropped::nodes.foldmoved to thepluginscrate asTreeNode::fold, with theunicode-normalizationdependency (fromui-egui, which no longer uses it elsewhere). The front end folds the query with the same function, so the two can't drift.TreeNode::newbuilds a node with its folded label; the field ispub(crate)so a node can't be made outside the crate with a stale one.Measurement
A throwaway test (not in the PR) over 20,000 nodes with municipality-like labels ("São José do Rio Preto 123", "Mãe d'Água 456"…), searching "sao jose", in the workspace's test profile (optimized + debuginfo), Windows, three runs:
Folding is now paid once per answer that sends the nodes: building the 20,000 nodes took 34 ms (63 ms in the noisy third run), the
format!of the test labels included. Before, a 20,000-node search would have spent about 30 ms of every frame on folding alone, past a 60 fps frame's 16 ms.Tests
a_tree_cut_at_its_limit_says_so(plugins): 20,000 nodes are all kept with nothing said; 20,001 keep 20,000 and say the cut once, naming the tree and the count, not again in the generic count; a nested tree cut inside a branch counts the branches after it with what they hold, and a node without an id is still counted apart; fewer nodes again clearcut.a_tree_search_reads_the_label_folded_once(plugins): labels with accents (composed and decomposed), and ids standing for a missing label, are folded when kept;holds_wordsreads the stored folded label (changing it changes what is found).tree_rows_follow_open_branches_and_searches(ui-egui) runs as before onTreeNode::new.Locally (Windows):
cargo fmt --check,cargo clippy -p rustgis-plugins -p rustgis-engine -p rustgis-ui-egui --all-targets -- -D warningsandcargo xtask layerspass; the tests ofrustgis-pluginsandrustgis-ui-eguipass. Inrustgis-enginetwo tests fail on this PC on unrelated code:gdal_formats_open_when_gdal_is_installed(the GDAL here is older than the 3.8 it asks for) andportable_projects::reload::wrong_geometry_missing_file_and_missing_chosen_table_leave_the_layer_unchanged, which expects "file" in the operating system's error and gets it in Portuguese here. Twoarchive_render_revisiontests failed once under load and passed when run again.unicode-normalizationis already in bothTHIRD-PARTY-LICENSES.txtfiles, through other crates, so the notices don't change.Limits
🤖 Generated with Claude Code