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

Added merging tolerance within LusasConfig to apply to both the comparers and the software #374

Merged

Conversation

KalleEdstroem
Copy link
Contributor

@KalleEdstroem KalleEdstroem commented Oct 16, 2023

Issues addressed by this PR

Closes #315

Added a feature to set the Merge Tolerance.
The property is set in the LusasConfig. and the merge tolerance gets set when the adapter is activated.

Test files

Initialize adapter with and without LusasConfig.

Versioning test script. Start without building and inspect. Build and inspect upgrades.

https://burohappold.sharepoint.com/:f:/r/sites/BHoM/02_Current/12_Scripts/02_Pull%20Request/BHoM/Lusas_Toolkit/%23315-ModifyMergingTolerance?csf=1&web=1&e=Szf9WG

Changelog

  • Added variable, MergeTolerance to the LusasConfig component. Added referance to Geometry_oM in LusasConfig to use Tolerance class for default value of MergeTolerance. Added description about the variable to the component.
  • The change lets you, with the LusasConfig, set a value for Merge Tolerance in Lusas upon opening a model with the LusasAdapter.

Additional comments

The default value is set to Tolerance.Distance .

For testing of versioning it is needed to build the Versioning_Toolkit after building this branch.

@KalleEdstroem KalleEdstroem added the type:feature New capability or enhancement label Oct 16, 2023
@KalleEdstroem
Copy link
Contributor Author

Should change mergingTolerance to mergeTolerance. To stay consistent with previous change made

Copy link
Member

@peterjamesnugent peterjamesnugent left a comment

Choose a reason for hiding this comment

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

Tested with the script and it works changing the merging tolerance in Lusas.

Have commented some changes below to implement.

Lusas_oM/Settings/LusasConfig.cs Outdated Show resolved Hide resolved
Lusas_Adapter/LusasAdapter.cs Outdated Show resolved Hide resolved
Lusas_Engine/Create/LusasConfig.cs Outdated Show resolved Hide resolved
@peterjamesnugent peterjamesnugent changed the title Lusas toolkit #315 add method modifying merging tolerance Added merging tolerance within LusasConfig to apply to both the comparers and the software Oct 17, 2023
Copy link
Member

@peterjamesnugent peterjamesnugent left a comment

Choose a reason for hiding this comment

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

Changes as discussed.

Lusas_oM/Lusas_oM.csproj Outdated Show resolved Hide resolved
Lusas_Adapter/LusasAdapter.cs Outdated Show resolved Hide resolved
Copy link
Member

@peterjamesnugent peterjamesnugent left a comment

Choose a reason for hiding this comment

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

Tested with Lusas v19, v19.1 and v20 using the test script.

Small change to give the m_mergingTolerance a default value when it is initialised.

Lusas_Adapter/LusasAdapter.cs Outdated Show resolved Hide resolved
Lusas_Adapter/LusasAdapter.cs Outdated Show resolved Hide resolved
@KalleEdstroem KalleEdstroem marked this pull request as draft November 3, 2023 10:39
Lusas_Engine/Create/LusasSettings.cs Outdated Show resolved Hide resolved
Lusas_Adapter/LusasAdapter.cs Outdated Show resolved Hide resolved
Lusas_Adapter/LusasAdapter.cs Outdated Show resolved Hide resolved
Lusas_Adapter/LusasAdapter.cs Outdated Show resolved Hide resolved
Lusas_Adapter/LusasAdapter.cs Outdated Show resolved Hide resolved
Lusas_Adapter/LusasAdapter.cs Outdated Show resolved Hide resolved
Copy link
Member

@peterjamesnugent peterjamesnugent left a comment

Choose a reason for hiding this comment

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

Two changes to descriptions

Lusas_oM/Versioning_70.json Outdated Show resolved Hide resolved
Lusas_oM/Settings/LusasSettings.cs Outdated Show resolved Hide resolved
@peterjamesnugent peterjamesnugent self-requested a review November 16, 2023 16:17
Copy link
Member

@peterjamesnugent peterjamesnugent left a comment

Choose a reason for hiding this comment

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

Happy with the changes, tested with the versioning script and test script.

Happy to accept that any internalised LusasConfig objects will not version - as they previously only contained a string for library settings, I highly doubt they were being used anywhere .

@peterjamesnugent
Copy link
Member

@BHoMBot check required

Copy link

bhombot-ci bot commented Nov 16, 2023

@peterjamesnugent to confirm, the following actions are now queued:

  • check code-compliance
  • check documentation-compliance
  • check project-compliance
  • check core
  • check null-handling
  • check serialisation
  • check versioning
  • check installer

@peterjamesnugent
Copy link
Member

@FraserGreenroyd can we get a neutral check on the versioning result please. There is a lot of Revit noise but one relevant to this PR:

    BH.oM.Adapters.Lusas.LusasConfig
    Error: Result returned as CustomObject
    Inner results:
        Failed to convert the string into a type: BH.oM.Adapters.Lusas.LusasConfig
        The type BH.oM.Adapters.Lusas.LusasConfig from version 6.3 is unknown -> data returned as custom objects.

Which is as intended due to the change from BHoMObject to IObject.

@FraserGreenroyd
Copy link
Contributor

@BHoMBot check versioning

Copy link

bhombot-ci bot commented Nov 16, 2023

@FraserGreenroyd to confirm, the following actions are now queued:

  • check versioning

@FraserGreenroyd
Copy link
Contributor

@BHoMBot check installer

Copy link

bhombot-ci bot commented Nov 16, 2023

@FraserGreenroyd to confirm, the following actions are now queued:

  • check installer

@FraserGreenroyd
Copy link
Contributor

@FraserGreenroyd can we get a neutral check on the versioning result please. There is a lot of Revit noise but one relevant to this PR:

    BH.oM.Adapters.Lusas.LusasConfig
    Error: Result returned as CustomObject
    Inner results:
        Failed to convert the string into a type: BH.oM.Adapters.Lusas.LusasConfig
        The type BH.oM.Adapters.Lusas.LusasConfig from version 6.3 is unknown -> data returned as custom objects.

Which is as intended due to the change from BHoMObject to IObject.

As discussed offline, dispensation for versioning checks aren't available because if a versioning issue is merged, it will then report failures on every pull request we have. The issue here was caused by the branch on Versioning_Toolkit still existing (despite PR being closed) from when we were looking at upgrading the object. I've deleted the branch over there and it looks like that's now fixed the versioning issues here 😄

@FraserGreenroyd
Copy link
Contributor

@BHoMBot check copyright-compliance

Copy link

bhombot-ci bot commented Nov 16, 2023

@FraserGreenroyd to confirm, the following actions are now queued:

  • check copyright-compliance

@FraserGreenroyd FraserGreenroyd dismissed their stale review November 16, 2023 19:04

My changes were made so all happy from my side.

@FraserGreenroyd
Copy link
Contributor

@BHoMBot check ready-to-merge

Copy link

bhombot-ci bot commented Nov 16, 2023

@FraserGreenroyd to confirm, the following actions are now queued:

  • check ready-to-merge

@FraserGreenroyd FraserGreenroyd merged commit 8a25b55 into develop Nov 16, 2023
10 checks passed
@FraserGreenroyd FraserGreenroyd deleted the Lusas_Toolkit-#315-AddMethodModifyingMergingTolerance branch November 16, 2023 19:06
@bhombot-ci bhombot-ci bot mentioned this pull request Dec 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:feature New capability or enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add method to modify merging tolerance in Lusas
3 participants