Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

UI: Fix performance of getting subject models in Models Tree #3666

Merged

Conversation

grigasp
Copy link
Member

@grigasp grigasp commented May 24, 2022

Fixes itwin/itwinjs-backlog#79

Doing the join in a single query takes up to a minute in the problematic iModel. Doing 2 separate queries and joining them on the frontend takes up to 200 ms.

@grigasp grigasp requested a review from a team as a code owner May 24, 2022 08:57
@grigasp grigasp requested a review from saskliutas May 24, 2022 09:00
@grigasp grigasp enabled auto-merge (squash) May 24, 2022 09:03
@grigasp
Copy link
Member Author

grigasp commented May 24, 2022

@Mergifyio backport release/2.19.x

@grigasp
Copy link
Member Author

grigasp commented May 24, 2022

@Mergifyio backport release/3.2.x

@mergify
Copy link
Contributor

mergify bot commented May 24, 2022

backport release/2.19.x

🟠 Waiting for conditions to match

  • merged [:pushpin: backport requirement]

@mergify
Copy link
Contributor

mergify bot commented May 24, 2022

backport release/3.2.x

🟠 Waiting for conditions to match

  • merged [:pushpin: backport requirement]

@grigasp grigasp merged commit 6924185 into master May 24, 2022
@grigasp grigasp deleted the ui/models-tree/fix-determining-subject-models-performance branch May 24, 2022 20:49
mergify bot pushed a commit that referenced this pull request May 24, 2022
@mergify
Copy link
Contributor

mergify bot commented May 24, 2022

backport release/3.2.x

✅ Backports have been created

mergify bot pushed a commit that referenced this pull request May 24, 2022
(cherry picked from commit 6924185)

# Conflicts:
#	ui/appui-react/src/appui-react/imodel-components/models-tree/ModelsVisibilityHandler.ts
#	ui/appui-react/src/test/imodel-components/models-tree/ModelsVisibilityHandler.test.ts
@mergify
Copy link
Contributor

mergify bot commented May 24, 2022

backport release/2.19.x

✅ Backports have been created

grigasp added a commit that referenced this pull request May 25, 2022
* UI: Fix performance of getting subject models (#3666)

(cherry picked from commit 6924185)

# Conflicts:
#	ui/appui-react/src/appui-react/imodel-components/models-tree/ModelsVisibilityHandler.ts
#	ui/appui-react/src/test/imodel-components/models-tree/ModelsVisibilityHandler.test.ts

* Fix merge

Co-authored-by: Grigas <35135765+grigasp@users.noreply.github.com>
};
const queryModels = (): AsyncIterableIterator<{ id: Id64String, parentId: Id64String, content?: string }> => {
const modelsQuery = `
SELECT p.ECInstanceId id, p.Parent.Id parentId, json_extract(p.JsonProperties, '$.PhysicalPartition.Model.Content') content
Copy link
Contributor

Choose a reason for hiding this comment

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

I've noticed that the "PhysicalPartition.Model.Content" Json name changes per type of Partition. That is, it can also be "SpatialLocationPartition.Model.Content" or "GeometricPartition3d.Model.Content". It seems the latter two will be missed by the "queryModels" ECSQL above.

I've also seen such piece of Json is not written by all connectors. At least the IFC and Revit connectors don't write it. Maybe only the dgn connectors do. It seems such piece of Json drives some "isHidden" attribute later on.

I believe we should revisit the reasons driving the introduction of such piece of Json and corresponding UX behaviors. Since it is not supported by all connectors, and it has problems with multiple kinds of partitions, it looks to be rather the source of bugs and inconsistencies in the UX.

cc: @grigasp

Copy link
Contributor

Choose a reason for hiding this comment

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

IIRC The BIS working group recommended the current design. The goal was to add a hint to a Subject element so that presentation rules could know what it is for and how or if to display it. That was preferred to adding an IsPrivate or IsHidden property to the Subject class.

We only need presentation hints like this because connectors create Subjects that are not relevant to the user.

Copy link
Member Author

Choose a reason for hiding this comment

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

We only need presentation hints like this because connectors create Subjects that are not relevant to the user.

We have so many problems because of the fact that Subjects serve multiple purposes... And, sadly, it seems we're not going to have a concept that we could directly use for creating the hierarchy. Because of that, we have a project to remove the Subjects hierarchy from the Models Tree altogether. The project was on hold lately, I hope we can revive it soon.

Copy link
Member Author

Choose a reason for hiding this comment

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

BTW, turns out querying the PhysicalPartition.Model.Content was unnecessary - here's a PR to remove it: #3697.

It's still used when creating the hierarchy, though. There we use PhysicalPartition.Model.Content and GraphicalPartition3d.Model.Content. Now you're saying we're missing a few others with a possibility that there might be even more than that. Sounds like the list is not definite, because we'd get a new attribute every time a a new subclass of InformationPartitionElement is introduced.

@swwilson-bsi says that the sole purpose of this flag is to help create the Models Tree. Then I'd question what's the point of creating so many different attributes and making it difficult to capture all of them for the only component that uses them.

And just to explain what the attribute does in the Models Tree... When an InformationPartitionElement has this attribute, we don't show its Model node and instead show Categories directly under Subject node. So not capturing the attribute would mean the Model node is displayed.

pmconne added a commit that referenced this pull request Jun 21, 2022
* Update changelog for 3.2.0 (#3613)

* grouping, TOC

* TOC, verbosity

* Add missing features.

* Update CI to test Node16 (#2987)

* Update CI to test Node16

* update doc references to Node 14
* update types/node version

* cleanup map-layers-auth
* Improved documentation in README.md

* get transformer cover to pass on windows, need to investigate more
* remove pretest script from transformer

Co-authored-by: Arun George <aruniverse@users.noreply.github.com>
Co-authored-by: Michel D'Astous <mdastous-bentley@users.noreply.github.com>

* 3.2.0-dev.73

* Multi-way viewport sync (#3614)

* multi-viewport sync.

* multi sync

* wip tests.

* wip more tests.

* finish tests.

* rename files.

* docs

* extract-api

* @extensions

* lint

* NextVersion

* clearer parameter name

* typo

* dta keyins can operate on more than 2 viewports.

* README

* inaccurate doc:

* 3.2.0-dev.74

* 3.3.0-dev.0

* close transformer state dumps even if overrides threw an error (#3612)

* close transformer state dumps even if producing them threw an error

Co-authored-by: Michael Belousov <MichaelBelousov@users.noreply.github.com>

* 3.3.0-dev.1

* 3.3.0-dev.2

* 3.3.0-dev.3

* Extensions: Avoid Webpack critical dependency warning using a FunctionConstructor (#3616)

* Avoid webpack critical dependency warning using FunctionConstructor

* rush change

* rush change (with changelog entry)

Co-authored-by: Caleb Shafer <31107829+calebmshafer@users.noreply.github.com>

* Fix a display issue with background color's ColorPicker where part of the dialog was missing. (#3617)

* Set default color input type to rgb on ColorPickerDialog

* Change log

* 3.3.0-dev.4

* update extension codeowners (#3630)

Co-authored-by: Arun George <aruniverse@users.noreply.github.com>

* Correctly remove a widget in FLOATING_WIDGET_SEND_BACK (#3619)

* Remove a widget in FLOATING_WIDGET_SEND_BACK.

* Rush change.

* Typo.

* [Extension Api]: Stable auto generated api (#3627)

* try to clean up and sort the generation script

Co-authored-by: Arun George <aruniverse@users.noreply.github.com>
Co-authored-by: Paul Connelly <22944042+pmconne@users.noreply.github.com>

* Catch snap abandoned exception. (#3634)

* 3.3.0-dev.5

* Presentation: Fix hierarchy auto update (#3628)

* Set correct children count after update

* rush change

* extract-api

* Presentation: Fix excluding related properties without values (#3639)

* 3.3.0-dev.6

* Add learning snippets for property specification isReadOnly and priority attributes (#3640)

Co-authored-by: Alina Paliulionyte <Alina657@users.noreply.github.com>

* Fix sizing of color picker dialog (#3623)

* Remove hardcoded height from color dialog.

* rush change

* UI: TypeConverter improvements (#3589)

* Return default number value instead of NaN

* rush change

* Return undefined instead of NaN

* rush change

* add isolate elements feature to transformer test app (#3601)

* add isolateElements tool to the transformer test-app

* add geometry options

* add to README note about the new isolatedElements feature

* fix cloneUsingJsonGeometry option description

Co-authored-by: Michael Belousov <MichaelBelousov@users.noreply.github.com>

* Prevent duplicate logo cards. (#3648)

* 3.3.0-dev.7

* Remove cached fronstageDef went FrontstageProvider with same id is re-registered (#3647)

* Remove cached frontstageDef if frontstageProvider is re-registered.

* Add test and change log.

* 3.2.0 changelogs (#3654)

* 3.2.0

* rush change

Co-authored-by: imodeljs-admin <38288322+imodeljs-admin@users.noreply.github.com>
Co-authored-by: Arun George <aruniverse@users.noreply.github.com>

* Compression branch (#3658)

* added compression

* added changelog

* Omit auth in RPC communication between mobile frontend and backend (#3649)

* Omit access token when mobile frontend talks to backend

* Changelog entry for mobile RPC auth omit

* Do not add backport reviewers if they have already approved the PR (#3659)

* Make display-test-app env vars use current values (#3644)

* change for env vars

* added shutdown to FrontEndDevTools and EditTool

* Fix drag target processing and update panel background color (#3635)

* Update to only confine floating widget to ninezone area on drag end and get proper widget/panel-section id when docking.

* Fix line height of overflow widget tabs when only icons are shown.

* Updates to avoid svg-loader component from loading svg multiple times.

* update unit test

* update panel background to use same as buic-background-control so dispabled itwin ui components display properly

* rush change

* update buic-background-panel to match buic-background-dialog

* fix lint error

* extract api

* Convert negative ViewRect coordinates to zero. (#3653)

* Convert negative ViewRect coordinates to zero.

* silly C++ holdover.

* missing paren in doc.

* 3.3.0-dev.8

* Valgrind azure pipeline (#3602)

* valgrind yaml build configuration

* updated yaml for valgrind

* git config for rush requirement

* try different rush cover call for valgrind

* Update valgrind.yaml for Azure Pipelines

* Update valgrind.yaml for Azure Pipelines

* Update valgrind.yaml for Azure Pipelines

* Update valgrind.yaml for Azure Pipelines

* Update valgrind.yaml for Azure Pipelines

* add suppression file for valgrind

* updated path to suppressions file

* switch to hosted agent

* switch to apt-get

* Increase timeout for valgrind tests

* Update valgrind.yaml for Azure Pipelines

* Update valgrind.yaml for Azure Pipelines

* Update valgrind.yaml for Azure Pipelines

* 3.3.0-dev.9

* 3.3.0-dev.10

* Upgrade mocha to latest version (10.0.0) (#3651)

* set mocha version for all projects to 10.0.0

Co-authored-by: Anmol Shrestha <anmolshres98@users.noreply.github.com>

* 3.3.0-dev.11

* Fix background map base color transparency mix problem (#3668)

* Reality mesh shader needs to mix base color with any overrides even if textures are not specified (map base color, for example, can be specified).

* rush change

* Fix schedule timeline by clamping current duration to specified range - closes issue #75 (#3673)

* Ensure current duration does not go out of range.

* rush change

* [Extension Service Provider] add option to override access token (#3665)

* add option to override access token to support dual tokens
Co-authored-by: Arun George <aruniverse@users.noreply.github.com>

* move valgrind runs to be nightly instead of every pr (#3676)

Co-authored-by: Arun George <aruniverse@users.noreply.github.com>

* UI: Fix performance of getting subject models (#3666)

* 3.3.0-dev.12

* Do not create a Batch with an empty FeatureTable (#3684)

* test degenerate facets.

* Fix.

* remove spurious type assertion.

* mac build agents are slow.

* increase default timeout

* 3.3.0-dev.13

* Fix assertion on valid case in IModelExporter.exportElement. (#3688)

* 3.3.0-dev.14

* Close popup when the widget tab is clicked (#3685)

* Allow tippy.js to close the popup when the tab is clicked.

* Prevent browser drag interaction when dragging a tab.

* Allow tippy.js to close the popup when the widget is resized.

* Rush change.

* Tests.

* 3.3.0-dev.15

* 3.3.0-dev.16

* 3.3.0-dev.17

* 3.3.0-dev.18

* Add workarounds for Mali-G72 GPU. (#3701)

Co-authored-by: Arun George <aruniverse@users.noreply.github.com>

* 3.3.0-dev.19

* Presentation: Update IModelContent ruleset (#3698)

* Update the hierarchy

* Fix geometric elements under geometric models not being loaded

* UI: Clean up unused code in Models Tree (#3697)

* Update itwinui-react to 1.38.1 (#3694)

* Update valgrind.yaml for Azure Pipelines (#3683)

* Fix initial pending request always being treated as an error. (#3660)

* Add caching to some RPC Operations (#3594)

* Initial commit

* can't use break when not in a loop oops

* rush change

* add test

* 23 hours for getTileCachecONtainerUrl

* fix ocmment

* Use the decorator for responsecaching in a test

* bump tile and read rpc interface versions

* approach cache-control header in a different way

* extrac-tapi

* extract-api

* remove @todo and just keep it as TODO

* remove .only

* Revert "approach cache-control header in a different way"

This reverts commit 9246de9.

* extract-api

Co-authored-by: Caleb Shafer <31107829+calebmshafer@users.noreply.github.com>

* Fix selection scope overrides (#3704)

* Ensure SelectionScope honor label overrides.

* rush change

* 3.3.0-dev.20

* Add OpenTelemetry tracing (#3548)

* add opentelemetry tracing

* rush update+change+extract

* fix dependencies

* remove unused import

* remove @opentelemetry/api dependency

* extract-api

* Refactor object flattening for OT span attributes.

* minor fixes

* WIP: Move OT support out of Logger.

* stop using deprecated types

* extract-api

* extract-api again

Co-authored-by: Bill Goehrig <33036725+wgoehrig@users.noreply.github.com>

* 3.3.0-dev.21

* 3.3.0-dev.22

* 3.3.0-dev.23

* 3.3.0-dev.24

* Add SchemaContext.getKnownSchemas method (#3734)

* Add SchemaContext.getKnkownSchemas method

* change log

* api extraction

* test fix

* api extract

* 3.3.0-dev.25

* 3.2.1 changelogs (#3757)

Co-authored-by: imodeljs-admin <38288322+imodeljs-admin@users.noreply.github.com>

* Ignore shader uniform errors by default (#3754)

* Ignore shader uniform errors by default

* Ignore shader uniform errors by default

* extract-api

* extract-api again

* Change to public

* enable logger for dta, doc new env

* Provide getter for active state (#3664)

* Provide getter for active state

Co-authored-by: Caleb Shafer <31107829+calebmshafer@users.noreply.github.com>

* 3.3.0-dev.26

* Remove Android / iOS Distinction in core-mobile. Remove AuthStatus from BentleyError and StatusCategory. (#3622)

* Remove Android / iOS Distinction in core-mobile. Remove AuthStatus from BentleyError and StatusCategory.

* UI: Fix `useTreeModel` returning stale model for the given model source (#3761)

* 3.3.0-dev.27

* Move LocalHub to core-backend (#3763)

* move LocalHub to @internal in core-backend

* rush change

* Add addCustomAttribute to EC Class and Property method to editing API (#3744)

* Add ability to add CA to Property in editing API

* wip

* change log

* remove it.only

* PR review updates

* PR updates

* fix lint errors

* Presentation: Add support for nth level element selection scopes (#3767)

* Move HubMock to core-backend (#3770)

* move HubMock to core-backend

* rush change

* 3.3.0-dev.28

* Presentation: Add a way to get presentation backend's version by setting a flag in request params (#3756)

* 3.2.2 changelogs (#3799)

Co-authored-by: imodeljs-admin <38288322+imodeljs-admin@users.noreply.github.com>

* [Extensions] ExtensionAdmin registerHost should accept hostnames and urls (#3631)

* ExtensionAdmin should accept hostnames and urls

* Remove the "_exists" method from the remote and extension service providers
* make iTwinId default to public on Service Extensions
* remove any cast and just log the error msg as is
* remove 'ftp' since its something we don't expect as input
* documentation fixes
* ServiceExtension props should not require a version - defaults to the latest
* attempt to parse the text response if body is null (happened during testing)

* 3.3.0-dev.29

* 3.3.0-dev.30

* 3.3.0-dev.31

* Change setImmediate to setTimeout (#3798)

* Lock cheerio version down (enzyme depedency) (#3800)

* Lock cheerio version down (enzyme depedency)

* lock down enzyme verion to 3.10.0

Co-authored-by: Caleb Shafer <31107829+calebmshafer@users.noreply.github.com>

* Presentation: Custom nodes (#3772)

* Add renderer for nodes with too many children

* Render custom nodes

* Update pnpm-lock.yaml

* pnpm conflict fix

* extract-api fix

* extract-api fix 2

* Imports fix

* Imports sorted

* Add option for user nodeRenderer

* indentation fix

* indentation fix 2

* Naming changes

* rush update

Co-authored-by: Saulius Skliutas <24278440+saskliutas@users.noreply.github.com>

* export brep geometry when requested in IModelExporter (#3801)

* add roundtrip brep geom test

* export brep data through IModelExporter when wantGeometry is true

Co-authored-by: Michael Belousov <MichaelBelousov@users.noreply.github.com>

* Create sphere and cylinder tools. Fixes for updateDynamics to supply adjusted point. (#3807)

* Create sphere and cylinder tools. Fixes for updateDynamics to supply ajusted point.

* Forgot to export new tool classes.

* 3.3.0-dev.32

* Clear drag state when button let up outside view. Don't send tools drag events if they didn't get button down event. (#3811)

* Fix tile display issues with ArcGIS map  layers (#3804)

* Make maplayers API public (#3745)

* Makes maplayers API public

* changelog

* Updated map-layers related API

* Promote more classes to public

* Added missing extract-api file.

* Expose MapLayerSource, MapLayerSourceProps, MapLayerSourceStatus, MapLayerSourceValidation

* Added MapLayerFormatId type.

* Replaced string enum with union type.

* Removed userName and password from MapLayerSourceProps

* Reverted formatId to string. Added MapLAyers group discussion. Use default parameters  in attachMapLayer and AttachMapLayerProps.

* Keep getMapLayerRange internal for now.

* Added change log.

* fix extract-api

* Use named parameters in DisplayTyleState.attachMapLayer and DisplayTyleState.attachMapLayerProps

* extract-api again

* 3.3.0-dev.33

* Use active tool name as tool settings label (#3814)

* Add useToolAsToolSettingsLabel option to UiFramework

* Extract api

* rush change

* 3.3.0-dev.34

* Presentation: Enable response compression for certain PresentationRpcInterface operations (#3818)

* 3.3.0-dev.35

* prevent Delaunay flipper from removing hole edges (#3731)

* Avoid slow keyin filtering when processing long keyins (#3830)

* Avoid doing slow filtering when not necessary

* revert change to filter

* rush change

* 3.3.0-dev.36

* how about we avoid running rush update on imodel02 branch.

* fix up core-backend

* fix CheckpointManager test.

* Delete core-transformer's HubMock

Temporarily comment out TileCache tests.

* unused imports.

* 3.3.0-dev.37

* fix TileCache.test.ts merge

* extract-api; doc TODOs

* lint

* invalid doc links.

* 3.3.0-dev.38

* Fix presentation-backend test

* Automated GitHub Releases (#3820)

* add python release script
* add release creation to version bump pipeline
* use gh action to create release

* remove unncessary dependencies from map-layers-auth

* re-remove merge-restored removed deepEqualWithFpTolerance impl

* Convert getModelProps RPC operation to a GET request from POST (#3835)

* change getModelProps to a GET request

* rush change

* Lock down ts-node to 10.8.0 for now because istanbuljs/nyc#1473

* fix slash direction

* Lock superagent down to 7.1.3 because 7.1.6 has core-full-stack-test integration test failures in electron.

* String.replaceAll no existe.

* Remove RPC preflight and InitializeInterface. (#3831)

* remove unnecessary pretest script that triggers an nyc bug

* 3.3.0-dev.39

* 3.3.0-dev.40

* fix test errors

* extract-api, lint

* remove temporary test setting for gcs

* fix lint error

Co-authored-by: Caleb Shafer <31107829+calebmshafer@users.noreply.github.com>
Co-authored-by: Arun George <aruniverse@users.noreply.github.com>
Co-authored-by: Michel D'Astous <mdastous-bentley@users.noreply.github.com>
Co-authored-by: imodeljs-admin <38288322+imodeljs-admin@users.noreply.github.com>
Co-authored-by: Michael Belousov <mike.belousov@bentley.com>
Co-authored-by: Michael Belousov <MichaelBelousov@users.noreply.github.com>
Co-authored-by: John DiMatteo <19596966+johnnyd710@users.noreply.github.com>
Co-authored-by: Arun George <11051042+aruniverse@users.noreply.github.com>
Co-authored-by: GerardasB <10091419+GerardasB@users.noreply.github.com>
Co-authored-by: bbastings <65233531+bbastings@users.noreply.github.com>
Co-authored-by: Saulius Skliutas <24278440+saskliutas@users.noreply.github.com>
Co-authored-by: Grigas <35135765+grigasp@users.noreply.github.com>
Co-authored-by: Alina Paliulionytė <67429235+Alina657@users.noreply.github.com>
Co-authored-by: Alina Paliulionyte <Alina657@users.noreply.github.com>
Co-authored-by: Bill Steinbock <65047615+bsteinbk@users.noreply.github.com>
Co-authored-by: ekandy <105654158+ekandy@users.noreply.github.com>
Co-authored-by: Travis Cobbs <77415528+tcobbs-bentley@users.noreply.github.com>
Co-authored-by: Seamus Kirby <32379572+skirby1996@users.noreply.github.com>
Co-authored-by: DStradley <48810710+DStradley@users.noreply.github.com>
Co-authored-by: Robert Schili <rschili@users.noreply.github.com>
Co-authored-by: Anmol Shrestha <98850418+anmolshres98@users.noreply.github.com>
Co-authored-by: Anmol Shrestha <anmolshres98@users.noreply.github.com>
Co-authored-by: Mark Schlosser <47000437+markschlosseratbentley@users.noreply.github.com>
Co-authored-by: Gytis Čepkauskas <98940208+GytisCepk@users.noreply.github.com>
Co-authored-by: Vykis <81580355+veekeys@users.noreply.github.com>
Co-authored-by: Bill Goehrig <33036725+wgoehrig@users.noreply.github.com>
Co-authored-by: nick4598 <22119573+nick4598@users.noreply.github.com>
Co-authored-by: Paulius Valiūnas <66480813+paulius-valiunas@users.noreply.github.com>
Co-authored-by: christophermlawson <32881725+christophermlawson@users.noreply.github.com>
Co-authored-by: Joe Zbuchalski <17307464+JoeZman@users.noreply.github.com>
Co-authored-by: Daniel Toby <daniel.e.toby@gmail.com>
Co-authored-by: kabentley <33296803+kabentley@users.noreply.github.com>
Co-authored-by: NancyMcCallB <45079789+NancyMcCallB@users.noreply.github.com>
Co-authored-by: Martynas <43886789+MartynasStrazdas@users.noreply.github.com>
Co-authored-by: Raphaël LEMIEUX <1904889+raplemie@users.noreply.github.com>
Co-authored-by: Robert Lukasonok <70327485+roluk@users.noreply.github.com>
Co-authored-by: dassaf4 <68340676+dassaf4@users.noreply.github.com>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Co-authored-by: kckst8 <6283674+kckst8@users.noreply.github.com>
Co-authored-by: AlainRobertAtBentley <73677355+AlainRobertAtBentley@users.noreply.github.com>
Co-authored-by: Daniel Toby <41296254+DanielToby@users.noreply.github.com>
Co-authored-by: naveedkhan8067 <38525837+naveedkhan8067@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants