fix(composer): submodel and hierarchy search fix #320
Conversation
sheilaXu
force-pushed
the
fix-hierarchy-search
branch
from
85df95f
to
669ebf4
Compare
| }: ISceneNodeInternal | Readonly<ISceneNodeInternal>) => { | ||
| const toSceneHeirarchyNode = ( | ||
| { ref, name, parentRef, childRefs = [], components }: ISceneNodeInternal | Readonly<ISceneNodeInternal>, | ||
| hideChild?: boolean, |
There was a problem hiding this comment.
I'm not sure this belongs here, this is kind of polluting the object model. Hiding children in search results is a view concern, it shouldn't affect the underlying data model.
Suggestion: We could move the filtering logic to the getChildNodes hook instead, and just make that aware of the search filters or similar.
There was a problem hiding this comment.
This might be the simplest way to achieve this though, so it's fine.
|
|
||
| const getChildNodes = useCallback( | ||
| (parentRef?: string) => { | ||
| const nodeMap = useStore(sceneComposerId).getState().document.nodeMap; | ||
| const results = Object.values(nodeMap) | ||
| .filter((node) => node.parentRef === parentRef) | ||
| .map(toSceneHeirarchyNode) | ||
| .map((node) => toSceneHeirarchyNode(node)) |
There was a problem hiding this comment.
This change creates an unnecessary wrapping function. Super minor, but it's technically another pointer the GC needs to clean up.
There was a problem hiding this comment.
the map function auto provide a number as second parameter, so if I don't wrap it, there will be error
| @@ -196,7 +196,7 @@ const SceneHierarchyDataProvider: FC<SceneHierarchyDataProviderProps> = ({ selec | |||
| <DndProvider backend={HTML5Backend}> | |||
| <Context.Provider | |||
| value={{ | |||
| rootNodes: rootNodes.map(toSceneHeirarchyNode), | |||
| rootNodes: rootNodes.map((node) => toSceneHeirarchyNode(node, !isEmpty(searchTerms))), | |||
There was a problem hiding this comment.
Looking at this, I would almost prefer to see a "filteredNodes" property passed through context instead, but we can save that for when we rework search properly.
There was a problem hiding this comment.
a filter callback? that's probably more scalable. i can have a try in my next change
| @@ -47,7 +48,8 @@ const SceneHierarchyTreeItem: FC<SceneHierarchyTreeItemProps> = ({ | |||
| (type as unknown as IModelRefComponentInternal)?.modelType !== ModelType.Environment, | |||
| ); | |||
|
|
|||
| const showSubModel = isValidModelRef && !!model && !isViewing(); | |||
| const [{ variation: subModelSelectionEnabled }] = useFeature(COMPOSER_FEATURES[COMPOSER_FEATURES.SubModelSelection]); | |||
There was a problem hiding this comment.
Can you just pass useFeature(COMPOSER_FEATURES.SubModelSelection)? Curious if the extra nesting is really necessary.
There was a problem hiding this comment.
I'm following how useFeature is used for other features, not quite sure why we did this.... probably because this COMPOSER_FEATURES enum used to be int enum not string enum..
| </React.Fragment> | ||
| ))} | ||
| {showSubModel && <SubModelTree parentRef={key} expanded={false} object3D={model!} selectable />} |
There was a problem hiding this comment.
Just a heads up that my incoming change will move this back into the React.Fragment with fixes for hierarchy placement. Non-blocking, but please let me know if you have any concerns with this regarding your fix.
There was a problem hiding this comment.
we shouldn't put it into the Fragment because this tree is for the root ModelRef node, not for the child SubModelRef nodes. the current code breaks submodel feature because when I create a new scene and add a model to it, I won't be able to see the tree and cannot add sub model ref component to the scene, so this tree never show up
Overview
search issue:
search-issue-viewing.mov
search fix:
search-fix-viewing.mov
sub-model issue:
submodel-issue.mov
sub-model fix:
submodel-fix.mov
Legal
This project is available under the Apache 2.0 License.