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

Add Priority Types For Push #393

Open
wants to merge 15 commits into
base: develop
Choose a base branch
from

Conversation

GCRA101
Copy link

@GCRA101 GCRA101 commented Oct 2, 2024

NOTE: Depends on

BHoM/ETABS_Toolkit#480

Issues addressed by this PR

Closes #390

BHoM_Adapter now equipped with a new attribute PropertyTypes defining priority order for the push of objects into software packages in addition to DependencyTypes. This change has been required due to the requirements from software packages like ETABS to push objects following a specific hierarchy.

Test files

Video Demonstration - Issue
https://burohappold.sharepoint.com/:v:/r/sites/BHoM/02_Current/12_Scripts/02_Pull%20Request/BHoM/ETABS_Toolkit/%23480-AddPriorityTypesForPush/Push%20Levels%20Issue.mp4?csf=1&web=1&e=7cEacd
Video Demonstration - Solution
https://burohappold.sharepoint.com/:v:/r/sites/BHoM/02_Current/12_Scripts/02_Pull%20Request/BHoM/ETABS_Toolkit/%23480-AddPriorityTypesForPush/Push%20Levels%20Sorted.mp4?csf=1&web=1&e=cDUbje
Json File
https://burohappold.sharepoint.com/sites/BHoM/_layouts/15/download.aspx?UniqueId=aa695dbddee44123b35aecc6843d658a&e=mvnMou
Grasshopper File
https://burohappold.sharepoint.com/:u:/r/sites/BHoM/02_Current/12_Scripts/02_Pull%20Request/BHoM/ETABS_Toolkit/%23480-AddPriorityTypesForPush/Test%20Script.gh?csf=1&web=1&e=Lzfzec
Etabs File
https://burohappold.sharepoint.com/:u:/r/sites/BHoM/02_Current/12_Scripts/02_Pull%20Request/BHoM/ETABS_Toolkit/%23480-AddPriorityTypesForPush/Test%20ETABS%20Model.EDB?csf=1&web=1&e=lj7JY7

Changelog

  • Added PriorityTypes attribute as dictionary associating priority number to each type of object to push
  • Added HandlePriorities attribute allowing Adapters to sort by priorities if required (e.g. ETABS)
  • Added PriorityComparer defining criteria for sorting objects based on priority
  • Added GetPrioritySortedObjects to sort objects based on priority and using the PriorityComparer

@GCRA101 GCRA101 self-assigned this Oct 2, 2024
@GCRA101 GCRA101 added type:bug Error or unexpected behaviour type:feature New capability or enhancement labels Oct 2, 2024
Copy link
Contributor

@IsakNaslundBh IsakNaslundBh left a comment

Choose a reason for hiding this comment

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

Good initial work @GCRA101 , but I think this can be simplified quite a bit.

Also, think this should be used for exceptional cases where the dependency sorting is not enough so you don't have to add all types in to the priority types, but just the ones that really needs to go before everything else, and is not already handled by the dependency sorting.

Please see comments.

Adapter_oM/IBHoMAdapter.cs Outdated Show resolved Hide resolved
Adapter_oM/PriorityComparer.cs Outdated Show resolved Hide resolved
BHoM_Adapter/BHoMAdapter.cs Outdated Show resolved Hide resolved
BHoM_Adapter/BHoM_Adapter.csproj Outdated Show resolved Hide resolved
Adapter_Engine/Query/GetPrioritySortedObjects.cs Outdated Show resolved Hide resolved
Adapter_oM/Settings-Config/AdapterSettings.cs Show resolved Hide resolved
BHoM_Adapter/AdapterActions/Push.cs Outdated Show resolved Hide resolved
BHoM_Adapter/AdapterActions/Push.cs Outdated Show resolved Hide resolved
@alelom
Copy link
Member

alelom commented Oct 3, 2024

Had a look at this PR and the ETABS one. Great work @GCRA101 ! 🚀
I agree with the changes suggested by @IsakNaslundBh .

GCRA101 and others added 9 commits October 16, 2024 14:00
Co-authored-by: Isak Näslund <isak.naslund@burohappold.com>
Co-authored-by: Isak Näslund <isak.naslund@burohappold.com>
After removal of PriorityComparer class

Co-authored-by: Isak Näslund <isak.naslund@burohappold.com>
… from Algorithm based on Dependencies

Co-authored-by: Isak Näslund <isak.naslund@burohappold.com>
Co-authored-by: Isak Näslund <isak.naslund@burohappold.com>
@GCRA101
Copy link
Author

GCRA101 commented Oct 16, 2024

@IsakNaslundBh, @alelom,
thanks again for the comments.
I've incorporated them in the latest commit 3587c92

@IsakNaslundBh IsakNaslundBh dismissed their stale review October 18, 2024 07:01

Requested changes now addressed and happy with this from a code perspective. Dismissing my request for change review

@IsakNaslundBh
Copy link
Contributor

Happy with this from a code perspective now, and have dismissed my review. @Chrisshort92 Could you please assist in testing this from a functionality point of view?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:bug Error or unexpected behaviour type:feature New capability or enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add PriorityTypes module for prioritising objects being pushed
3 participants