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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 2 commits into from
May 24, 2022
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0441289
Fix performance of getting subject models
grigasp May 24, 2022
78016ae
Merge branch 'master' into ui/models-tree/fix-determining-subject-mod…
calebmshafer May 24, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions ...pui-react/ui-models-tree-fix-determining-subject-models-performance_2022-05-24-08-55.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
{
"changes": [
{
"packageName": "@itwin/appui-react",
"comment": "Models Tree: Fix performance of determining Subject nodes' display state.",
"type": "none"
}
],
"packageName": "@itwin/appui-react"
}
67 changes: 41 additions & 26 deletions ui/appui-react/src/appui-react/imodel-components/models-tree/ModelsVisibilityHandler.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -419,43 +419,58 @@ class SubjectModelIdsCache {
this._imodel = imodel;
}

private async initSubjectsHierarchy() {
this._subjectsHierarchy = new Map();
const ecsql = `SELECT ECInstanceId id, Parent.Id parentId FROM bis.Subject WHERE Parent IS NOT NULL`;
const result = this._imodel.query(ecsql, undefined, { rowFormat: QueryRowFormat.UseJsPropertyNames });
for await (const row of result) {
let list = this._subjectsHierarchy.get(row.parentId);
private async initSubjectModels() {
const querySubjects = (): AsyncIterableIterator<{ id: Id64String, parentId?: Id64String, targetPartitionId?: Id64String }> => {
const subjectsQuery = `
SELECT ECInstanceId id, Parent.Id parentId, json_extract(JsonProperties, '$.Subject.Model.TargetPartition') targetPartitionId
FROM bis.Subject
`;
return this._imodel.query(subjectsQuery, undefined, { rowFormat: QueryRowFormat.UseJsPropertyNames });
};
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.

FROM bis.InformationPartitionElement p
INNER JOIN bis.GeometricModel3d m ON m.ModeledElement.Id = p.ECInstanceId
WHERE NOT m.IsPrivate
`;
return this._imodel.query(modelsQuery, undefined, { rowFormat: QueryRowFormat.UseJsPropertyNames });
};

function pushToMap<TKey, TValue>(map: Map<TKey, TValue[]>, key: TKey, value: TValue) {
let list = map.get(key);
if (!list) {
list = [];
this._subjectsHierarchy.set(row.parentId, list);
map.set(key, list);
}
list.push(row.id);
list.push(value);
}

this._subjectsHierarchy = new Map();
const targetPartitionSubjects = new Map<Id64String, Id64String[]>();
for await (const subject of querySubjects()) {
if (subject.parentId)
pushToMap(this._subjectsHierarchy, subject.parentId, subject.id);
if (subject.targetPartitionId)
pushToMap(targetPartitionSubjects, subject.targetPartitionId, subject.id);
}
}

private async initSubjectModels() {
this._subjectModels = new Map();
const ecsql = `
SELECT p.ECInstanceId id, s.ECInstanceId subjectId, json_extract(p.JsonProperties, '$.PhysicalPartition.Model.Content') content
FROM bis.InformationPartitionElement p
INNER JOIN bis.GeometricModel3d m ON m.ModeledElement.Id = p.ECInstanceId
INNER JOIN bis.Subject s ON (s.ECInstanceId = p.Parent.Id OR json_extract(s.JsonProperties, '$.Subject.Model.TargetPartition') = printf('0x%x', p.ECInstanceId))
WHERE NOT m.IsPrivate`;
const result = this._imodel.query(ecsql, undefined, { rowFormat: QueryRowFormat.UseJsPropertyNames });
for await (const row of result) {
let list = this._subjectModels.get(row.subjectId);
if (!list) {
list = [];
this._subjectModels.set(row.subjectId, list);
}
const isHidden = row.content !== undefined;
list.push({ id: row.id, isHidden });
for await (const model of queryModels()) {
const subjectIds = targetPartitionSubjects.get(model.id) ?? [];
if (!subjectIds.includes(model.parentId))
subjectIds.push(model.parentId);

const v = { id: model.id, isHidden: (model.content !== undefined) };
subjectIds.forEach((subjectId) => {
pushToMap(this._subjectModels!, subjectId, v);
});
}
}

private async initCache() {
if (!this._init) {
this._init = Promise.all([this.initSubjectModels(), this.initSubjectsHierarchy()]).then(() => { });
this._init = this.initSubjectModels().then(() => { });
}
return this._init;
}
Expand Down
4 changes: 2 additions & 2 deletions ui/appui-react/src/test/imodel-components/models-tree/ModelsVisibilityHandler.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,8 +104,8 @@ describe("ModelsVisibilityHandler", () => {
});
props.imodelMock.setup((x) => x.query(moq.It.is((q: string) => (-1 !== q.indexOf("FROM bis.InformationPartitionElement"))), undefined, { rowFormat: QueryRowFormat.UseJsPropertyNames }))
.returns(async function* () {
const list = new Array<{ id: Id64String, subjectId: Id64String, content?: string }>();
props.subjectModels.forEach((modelInfos, subjectId) => modelInfos.forEach((modelInfo) => list.push({ id: modelInfo.id, subjectId, content: modelInfo.content })));
const list = new Array<{ id: Id64String, parentId: Id64String, content?: string }>();
props.subjectModels.forEach((modelInfos, subjectId) => modelInfos.forEach((modelInfo) => list.push({ id: modelInfo.id, parentId: subjectId, content: modelInfo.content })));
while (list.length)
yield list.shift();
});
Expand Down