[Tree widget]: Add docs for visibility handling#1606
[Tree widget]: Add docs for visibility handling#1606JonasDov wants to merge 13 commits intotree-widget/nextfrom
Conversation
There was a problem hiding this comment.
Tree-Widget Next benchmark
| Benchmark suite | Current: 2eb2e5f | Previous: 2af5df9 | Deviation | Status |
|---|---|---|---|---|
categories tree creates initial filtered view for 50k items |
1538.04 ms |
1549.79 ms |
-0.76% |
〰️ |
categories tree creates initial filtered view for 50k items (P95 of main thread blocks) |
166 ms |
159 ms |
4.40% |
〰️ |
categories tree changing definition container visibility changes visibility for 50k subCategories |
4759.2 ms |
4571.77 ms |
4.10% |
〰️ |
categories tree changing definition container visibility changes visibility for 50k subCategories (P95 of main thread blocks) |
63 ms |
57 ms |
10.53% |
〰️ |
categories tree changing definition container visibility changes visibility for 50k categories |
11629.78 ms |
11403.6 ms |
1.98% |
〰️ |
categories tree changing definition container visibility changes visibility for 50k categories (P95 of main thread blocks) |
405 ms |
406 ms |
-0.25% |
〰️ |
classifications tree loads initial view for iModel with 50k classifications |
43.41 ms |
44.22 ms |
-1.83% |
〰️ |
classifications tree loads initial view for iModel with 50k classifications (P95 of main thread blocks) |
22 ms |
22 ms |
0% |
🟰 |
classifications tree loads first branch for iModel with 50k classifications |
1061.59 ms |
1118.11 ms |
-5.05% |
〰️ |
classifications tree loads first branch for iModel with 50k classifications (P95 of main thread blocks) |
128 ms |
151 ms |
-15.23% |
〰️ |
models tree creates initial filtered view for 50k target items |
933.7 ms |
991.55 ms |
-5.83% |
〰️ |
models tree creates initial filtered view for 50k target items (P95 of main thread blocks) |
85 ms |
122 ms |
-30.33% |
〰️ |
models tree validates categories visibility for imodel with 50k categories |
5614.92 ms |
5778.18 ms |
-2.83% |
〰️ |
models tree validates categories visibility for imodel with 50k categories (P95 of main thread blocks) |
282 ms |
284 ms |
-0.70% |
〰️ |
models tree changing model visibility changes visibility for 50k elements |
2652.94 ms |
2974.55 ms |
-10.81% |
✅ |
models tree changing model visibility changes visibility for 50k elements (P95 of main thread blocks) |
0 ms |
22 ms |
-100% |
〰️ |
models tree changing category visibility changes visibility for 50k elements |
2681.01 ms |
3005.03 ms |
-10.78% |
✅ |
models tree changing category visibility changes visibility for 50k elements (P95 of main thread blocks) |
0 ms |
0 ms |
0% |
🟰 |
models tree changing per-model-category override changes visibility for 50k elements |
2614.94 ms |
3030.66 ms |
-13.72% |
✅ |
models tree changing per-model-category override changes visibility for 50k elements (P95 of main thread blocks) |
0 ms |
0 ms |
0% |
🟰 |
models tree changing element visibility changes only parent nodes visibility with 50k elements |
3602.19 ms |
3847.48 ms |
-6.38% |
〰️ |
models tree changing element visibility changes only parent nodes visibility with 50k elements (P95 of main thread blocks) |
466 ms |
459 ms |
1.53% |
〰️ |
models tree changing element visibility changes only parent nodes visibility with 50k child elements with different categories |
3734.83 ms |
4025.93 ms |
-7.23% |
〰️ |
models tree changing element visibility changes only parent nodes visibility with 50k child elements with different categories (P95 of main thread blocks) |
409 ms |
350 ms |
16.86% |
🚨 |
This comment was automatically generated by workflow using github-action-benchmark.
|
what do you think about using this? |
|
|
||
| This document explains how visibility handling works across tree types (Models, Categories, and Classifications) and node types (models, categories, geometric elements, sub-categories, sub-models, classifications, classification tables and definition containers). | ||
|
|
||
| ## File Overview |
There was a problem hiding this comment.
This looks like an internal API rather than file overview. E.g. "Returns a tree-specific visibility handler" applies to a function rather than a file. So I'd rename the section and reference important internal APIs instead of files.
There was a problem hiding this comment.
Tried to update have a look.
| - Applies special handling when search paths are present (for nodes that are not search targets or have no search-target ancestors). | ||
|
|
||
| - [BaseVisibilityHelper.ts](./src/tree-widget-react/components/trees/common/internal/visibility/BaseVisibilityHelper.ts) | ||
| - Shared read/write operations for visibility based on element/model/category ids. |
There was a problem hiding this comment.
what does "read/write" mean in this context? maybe it should be renamed to "get/change"?
| - Uses `BaseIdsCache` to retrieve information about nodes. | ||
| - Examples: `getModelsVisibilityStatus()`, `getCategoriesVisibilityStatus()`, `changeModelsVisibilityStatus()`, `changeCategoriesVisibilityStatus()`. | ||
|
|
||
| - Tree-specific visibility handlers: |
There was a problem hiding this comment.
I think it's important to note here that these handlers are aware of the specific hierarchy structure.
There was a problem hiding this comment.
Added note These handlers are aware of tree-specific hierarchy structure.
|
|
||
| - [ElementChildrenCache.ts](./src/tree-widget-react/components/trees/common/internal/caches/ElementChildrenCache.ts) | ||
| - Cache for retrieving elements' children. | ||
| - Using this cache to query data is a little expensive, so it is only used when changing visibility for element or element grouping nodes. |
There was a problem hiding this comment.
This is a bit vague:
- Why is it expensive?
- Sounds like we need it for other cases, too, but because it's expensive we're not using it there - what does that mean? Are we ignoring elements' children in those cases? Or do we have other means to handle elements' children there?
There was a problem hiding this comment.
Updated the comment, hope it explains what I meant
| The viewport only renders elements. Element visibility is resolved in the following order (highest priority first): | ||
|
|
||
| 1. **Model selector**: if a model is hidden, its elements are never visible. | ||
| 2. **Always/Never drawn sets**: elements in these sets are forced to be visible/hidden. |
There was a problem hiding this comment.
I think it's worth mentioning the special "exclusive" flag here, too
There was a problem hiding this comment.
Added here and also in TreeWidgetViewport
| 4. **Category selector**: hidden categories hide their elements. | ||
| 5. **Sub-categories**: hidden sub-categories hide their elements. | ||
|
|
||
| Note: Determining element -> sub-category relationships is not supported at the moment. So sub-category checks are only performed when the Categories tree calls `getVisibilityStatus()` for categories or sub-categories. |
There was a problem hiding this comment.
I'd nest this under "Sub-categories" list item above.
| R3[/hidden/] | ||
|
|
||
| %% Start | ||
| TITLE([getSubjectsVisibilityStatus]) --> A["Get models under Props.subjectIds from cache. These are related models and models of child subjects (can be nested)"] |
There was a problem hiding this comment.
could we put Props.subjectIds to backticks?
There was a problem hiding this comment.
Seems that mermaid diagrams don't understand backticks and show backtick symbols instead. Wrapped in <code> sections instead.
| PROPS[\"Props | ||
| <div style='text-align: left;'>- subjectIds: **Id64Arg**</div> | ||
| "\] |
There was a problem hiding this comment.
For me it would be clearer if Props started with lower case, e.g. props. Interesting in hearing others' thoughts (@saskliutas @MartynasStrazdas)
| R1[/partial/] | ||
| R2[/visible/] | ||
| R3[/hidden/] |
There was a problem hiding this comment.
maybe change variable names to RESULT_Partial, RESULT_Visible and RESULT_Hidden?
| ### Getting visibility status | ||
|
|
||
| #### getSubjectsVisibilityStatus |
There was a problem hiding this comment.
What do you think about structuring them based on the tree they're used in, instead of just having a plain list of diagrams?
Here's what I'm imagining:
- Have a separate markdown file per tree + one for shared visibility handling.
- Each tree specific markdown would have its specific diagrams (e.g.
getSubjectsVisibilityStatuswould be in models tree markdown) and reference the shared ones that apply to them (e.g.getModelsVisibilityStatuswould be in shared markdown and referenced from models tree markdown).
- Each tree specific markdown would have its specific diagrams (e.g.
- The "Architecture" section stays, but instead of having all the diagrams it only references them.
- Each file will have less diagrams, so we can provide a bit more details. E.g. I think it would be good to have a short description of the diagram. Don't need much for the ones that are clear (e.g.
getSubjectsVisibilityStatus), but some of them aren't that clear (e.g.getModelWithCategoryVisibilityStatus) and could use an explanation.
There was a problem hiding this comment.
Tried to split up based on your suggestion, added short descriptions. Have a look.
There was a problem hiding this comment.
Pull request overview
This PR adds comprehensive documentation for visibility handling in the tree widget component. The documentation explains how visibility is determined and changed across different tree types (Models, Categories, and Classifications) and node types, providing detailed architectural insights with Mermaid flowcharts.
Changes:
- Added
Visibility.mddocumentation file with detailed architecture explanations and Mermaid flowcharts showing visibility status determination for different node types - Added Mermaid Chart VS Code extension to recommended extensions for easier diagram editing
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| packages/itwin/tree-widget/Visibility.md | New comprehensive documentation file explaining visibility handling architecture, including file structure overview, viewport visibility rules, and detailed flowcharts for visibility status methods |
| .vscode/extensions.json | Added mermaidchart.vscode-mermaid-chart extension to support viewing and editing the Mermaid diagrams in the new documentation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 9 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Added
Visibility.mdfile, it explains the structure of how visibility handling works in tree widget. Also, added diagrams that showcase how visibility status is determined for different types of nodes.