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

Allow Multiple Logic Requests #474

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

Conversation

GCRA101
Copy link
Contributor

@GCRA101 GCRA101 commented Sep 17, 2024

Issues addressed by this PR

Closes #465

GH Script Canvas View

ETABS Toolkit now can allow to pull objects from ETABS based on FilterRequests, SelectedRequest and multiple LogicalRequests.

Test files

Grasshopper File
https://burohappold.sharepoint.com/:u:/r/sites/BHoM/02_Current/12_Scripts/02_Pull%20Request/BHoM/ETABS_Toolkit/%23474-AllowMultipleLogicRequests/[TestScript.gh](https://burohappold.sharepoint.com/:u:/r/sites/BHoM/02_Current/12_Scripts/02_Pull%20Request/BHoM/ETABS_Toolkit/%23474-AllowMultipleLogicRequests/TestScript.gh?csf=1&web=1&e=93kD0Q)?csf=1&web=1&e=93kD0Q
ETABS File
https://burohappold.sharepoint.com/:u:/r/sites/BHoM/02_Current/12_Scripts/02_Pull%20Request/BHoM/ETABS_Toolkit/%23474-AllowMultipleLogicRequests/Test%20ETABS%20Model.EDB?csf=1&web=1&e=IRUEie

Changelog

  • Added Read Method for FitlerRequests
  • Added Read Method for LogicalRequests (And, Or and Not)
  • Added Dynamic Comparer class allowing to compare IBHoMObjects based on ad-hoc criteria

@GCRA101 GCRA101 self-assigned this Sep 17, 2024
@GCRA101 GCRA101 added this to the BHoM 7.4 β MVP milestone Sep 17, 2024
@GCRA101 GCRA101 added the type:feature New capability or enhancement label Sep 17, 2024
@GCRA101
Copy link
Contributor Author

GCRA101 commented Sep 18, 2024

@Chrisshort92 , @peterjamesnugent , @IsakNaslundBh,

I've had the chance to finish the pull request for issue #465.

Also for this one, a pull request to be reviewed in the next sprint moving forward BHoM 7.4, not now ;)

Thanks guys! ;)

Chrisshort92
Chrisshort92 previously approved these changes Oct 14, 2024
Copy link
Contributor

@Chrisshort92 Chrisshort92 left a comment

Choose a reason for hiding this comment

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

Functionality tested with files provided, all working as expected. Approved for merge

@Chrisshort92
Copy link
Contributor

@BHoMBot Check required

Copy link

bhombot-ci bot commented Oct 14, 2024

@Chrisshort92 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

There are 39 requests in the queue ahead of you.

Turned the getEtabsId() method name (from the DynamicComparer private class) into GetEtabsId(), based on BHoM method's naming convention.
@GCRA101
Copy link
Contributor Author

GCRA101 commented Oct 14, 2024

@Chrisshort92 , I've gone through the code compliance checks above and I've modified the code as per following commit 655b110
Turned the DynamicComparer from a private class within the _Read.cs file into a public class within the Types namespace.

@GCRA101
Copy link
Contributor Author

GCRA101 commented Oct 14, 2024

@BHoMBot Check required

Copy link

bhombot-ci bot commented Oct 14, 2024

@GCRA101 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

There are 85 requests in the queue ahead of you.

Copy link

bhombot-ci bot commented Oct 14, 2024

The check core has already been run previously and recorded as a successful check. This check has not been run again at this time.

Copy link

bhombot-ci bot commented Oct 14, 2024

The check serialisation has already been run previously and recorded as a successful check. This check has not been run again at this time.

Copy link

bhombot-ci bot commented Oct 14, 2024

The check versioning has already been run previously and recorded as a successful check. This check has not been run again at this time.

Copy link

bhombot-ci bot commented Oct 14, 2024

The check installer has already been run previously and recorded as a successful check. This check has not been run again at this time.

@Chrisshort92 Chrisshort92 self-requested a review October 14, 2024 13:31
Chrisshort92
Chrisshort92 previously approved these changes Oct 14, 2024
Copy link
Contributor

@Chrisshort92 Chrisshort92 left a comment

Choose a reason for hiding this comment

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

Re-tested code changes from @GCRA101, functionality working as expected, approved for merge.

@peterjamesnugent
Copy link
Member

@BHoMBot check compliance
@BHoMBot check ready-to-merge

Copy link

bhombot-ci bot commented Oct 23, 2024

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

  • check code-compliance
  • check documentation-compliance
  • check project-compliance
  • check branch-compliance
  • check dataset-compliance
  • check copyright-compliance
  • check ready-to-merge

There are 19 requests in the queue ahead of you.

Copy link

bhombot-ci bot commented Oct 23, 2024

The check code-compliance has already been run previously and recorded as a successful check. This check has not been run again at this time.

Copy link

bhombot-ci bot commented Oct 23, 2024

The check documentation-compliance has already been run previously and recorded as a successful check. This check has not been run again at this time.

Copy link

bhombot-ci bot commented Oct 23, 2024

The check project-compliance has already been run previously and recorded as a successful check. This check has not been run again at this time.

@GCRA101
Copy link
Contributor Author

GCRA101 commented Oct 24, 2024

@peterjamesnugent,
added copyright header to DynamicComparer in latest commit.
9b081de

@GCRA101
Copy link
Contributor Author

GCRA101 commented Oct 28, 2024

@BHoMBot check required

Copy link

bhombot-ci bot commented Oct 28, 2024

@GCRA101 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

@GCRA101
Copy link
Contributor Author

GCRA101 commented Oct 28, 2024

@BHoMBot check copyright-compliance

Copy link

bhombot-ci bot commented Oct 28, 2024

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

  • check copyright-compliance

There are 28 requests in the queue ahead of you.

@GCRA101
Copy link
Contributor Author

GCRA101 commented Oct 28, 2024

@BHoMBot check required

Copy link

bhombot-ci bot commented Oct 28, 2024

@GCRA101 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

There are 14 requests in the queue ahead of you.

Copy link

bhombot-ci bot commented Oct 28, 2024

Please be advised that the check with reference 32166477143 has more than 50 annotations of notes. API limitations restrict annotations to 50. You may need to rerun this check to obtain the next set when you make changes. At the time of reporting this check, there are 1318 additional annotations waiting, made up of 1318 errors and 0 warnings.

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.

Recurring theme of unchanged files popping up in the PRs, can we make sure they are removed.

On the DynamicComparer I feel like we can just use the comparers defined in the adatper rather than trying to rewrite those.

Etabs_Adapter/Convert/Execute.cs Outdated Show resolved Hide resolved
Comment on lines +55 to +62
// For physical objects, use Id as it is never null
if (objType == typeof(Node) || objType == typeof(Bar) || objType == typeof(Panel) || objType == typeof(RigidLink) || objType == typeof(FEMesh))
return GetEtabsId(obj1).Id.ToString() == GetEtabsId(obj2).Id.ToString();
// For all other items, use Name as it is never null
if (objType == typeof(ISectionProperty) || objType == typeof(IMaterialFragment) || objType == typeof(ISurfaceProperty) ||
objType == typeof(Loadcase) || objType == typeof(LoadCombination) || objType == typeof(ILoad) || objType == typeof(LinkConstraint) ||
objType == typeof(Level) || objType == typeof(Grid))
return obj1.Name.ToString() == obj2.Name.ToString();
Copy link
Member

Choose a reason for hiding this comment

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

We already have all these comparers defined in the Adapter - can we not make sure of that rather than rewriting it here:

{typeof(Node), new BH.Engine.Structure.NodeDistanceComparer(3) },
{typeof(Bar), new BH.Engine.Structure.BarEndNodesDistanceComparer(3) },
{typeof(ISectionProperty), new NameOrDescriptionComparer() },
{typeof(IMaterialFragment), new NameOrDescriptionComparer() },
{typeof(ISurfaceProperty), new NameOrDescriptionComparer() },
{typeof(BH.oM.Adapters.ETABS.Elements.Diaphragm), new BHoMObjectNameComparer() },

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@peterjamesnugent, thanks for the comment.
It would be great to reuse it yeah.
Only thing is that there are some objects that are not dealt with by the Adapter Comparer (e.g. Level, FEMesh,LoadCase...) - see screenshot below.
Can we add them in it?

image

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I don't think that's an issue adding them to the Comparer.cs - most of them will be NameOrDescriptionComparer() anyway.

Can you add to the test script, trying to push two objects with the same name for each type you add (just to test that we are not breaking anything).

@Chrisshort92
Copy link
Contributor

@BHoMBot check installer

Copy link

bhombot-ci bot commented Nov 25, 2024

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

  • check installer

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.

Allow for Multiple Logical Requests
3 participants