Skip to content

Clash sets: select only the named sets, and reject a set no model has - #144

Open
menes97 wants to merge 1 commit into
IOExtend:mainfrom
menes97:fix/clash-missing-set
Open

menes97 wants to merge 1 commit into
IOExtend:mainfrom
menes97:fix/clash-missing-set

Conversation

@menes97

@menes97 menes97 commented Nov 12, 2026

Copy link
Copy Markdown
Contributor

Fixes #52

ResolveSelection returned every mesh of the document when the named sets resolved to no meshes. A misspelled set name, a set whose nodes have no meshes, or, in an assembly, a set that exists in only one model all ran against the whole model without notice.

Following the issue's suggestions:

  • The SetNames branch always returns its matches, even when there are none, like the ExplicitNodeIds and FilterCondition branches. A model of an assembly that lacks the set adds no meshes, and only NwcClashSelection.All() selects every mesh.
  • Both NwcClashDetector.Run overloads throw an ArgumentException that names the test and the set when the set is in none of the models (the document, or every model of the assembly), before resolving anything. A set that exists in only some models of an assembly doesn't throw. The XML docs of both overloads mention the exception.

NwcDocument.RunClashDetection and NwcAssembly.RunClashDetection go through Run, so they throw the same way. The CLI doesn't build set selections, so it isn't affected.

Tests (ClashDetectiveTests), each on documents whose meshes clash:

  • SetSelection_UnknownSetName_Throws
  • SetSelection_SetWithoutMeshes_SelectsNothing: a set of a node id with no mesh gives no clashes.
  • SetSelection_SetInOneModelOnly_SelectsNothingFromTheOthers: the set is only in the first model of an assembly, and the second model adds nothing.
  • SetSelection_SetMissingFromEveryModel_Throws

All four fail without the change. dotnet build NwcIO.slnx succeeds, and NwcIO.Tests passes locally: 243 passed, and 2 skipped because they need private Navisworks files (.NET 10.0.400 on Windows, with global.json pinned down to it temporarily; not committed).

🤖 Generated with Claude Code

ResolveSelection returned every mesh of the document when the named sets
resolved to nothing, so a misspelled set name, a set without meshes, or
a set present in only one model of an assembly ran against the whole
model without notice. The set branch now returns its matches even when
they are empty, like the node id and filter branches, and Run throws an
ArgumentException naming a set that none of the models contains.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@mahach666 mahach666 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Changes requested: the intended fix is useful, but the inline comment below describes a regression reproduced against the reviewed head and the unchanged base. Please address it and add the indicated regression test before merging.

Finding: Validate set names only when that selection branch is active.

{
foreach (var selection in new[] { test.SelectionA, test.SelectionB })
{
foreach (string name in selection.SetNames)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[P2] Validate set names only when that selection branch is active

ResolveSelection gives ExplicitNodeIds precedence over FilterCondition and SetNames, but this new validation checks every set name unconditionally.

Reproduction: set SelectionA.ExplicitNodeIds = [1] (via Add/AddRange) and leave SelectionA.SetNames = ["unused missing set"]. With two overlapping meshes, the current main/base completes and returns one clash because the set-name branch is unused; this PR throws ArgumentException before resolving the explicit IDs. The same problem applies when FilterCondition overrides set names.

Please validate SetNames only when neither explicit IDs nor a filter condition determines the selection, and add a regression test for the existing precedence.

This branch has not been deployed

No deployments
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.

Clash selection by set name falls back to the whole model when the set is missing or resolves to nothing

2 participants