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.

Sign up for GitHub

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

Jump to bottom

Add absolute distances BarVaryingDistributedLoad #386

Merged
merged 9 commits into from
Sep 6, 2024
Merged

Add absolute distances BarVaryingDistributedLoad #386

merged 9 commits into from
Sep 6, 2024

Conversation

EmmaSander
Copy link
Contributor

@EmmaSander EmmaSander commented Aug 29, 2024 •
edited by peterjamesnugent
Loading

Issues addressed by this PR

Closes #270

Test files

https://burohappold.sharepoint.com/:u:/s/BHoM/EcypvhJiU-xChY8dJfVL-8sBTSXaNxMPg5VcWCp9IGC-8w?e=Mdwfdm

Changelog

  • Added support for BarVaryingDistributedLoad with absolute distances.

Additional comments

@EmmaSander EmmaSander added good first issue Good for newcomers type:feature New capability or enhancement labels Aug 29, 2024
@EmmaSander EmmaSander added this to the BHoM 7.3 β MVP milestone Aug 29, 2024
@EmmaSander EmmaSander self-assigned this Aug 29, 2024
peterjamesnugent
peterjamesnugent requested changes Sep 3, 2024
View reviewed changes
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.

Good start @EmmaSander, it does need some changes to capture the scenario where the Bar.Length is different.

MidasCivil_Adapter/CRUD/Create/Loads/BarVaryingDistributedLoads.cs Outdated Show resolved Hide resolved
MidasCivil_Adapter/Convert/ToMidasCivil/Loads/FromBarVaryingDistributedLoad.cs Outdated Show resolved Hide resolved
MidasCivil_Adapter/Convert/ToMidasCivil/Loads/FromBarVaryingDistributedLoad.cs Outdated Show resolved Hide resolved
MidasCivil_Adapter/CRUD/Create/Loads/BarVaryingDistributedLoads.cs Outdated Show resolved Hide resolved
@EmmaSander
Update FromBarVaryingDistributedLoad.cs
58f9039 
Cleaned up code
@EmmaSander
Update BarVaryingDistributedLoads.cs
fb85b87 
New and moved solution
@EmmaSander
Update FromBarVaryingDistributedLoad.cs
b5c5721 
Moved solution and added support for that
peterjamesnugent
peterjamesnugent requested changes Sep 5, 2024
View reviewed changes
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.

I have added some comments to be addressed.

MidasCivil_Adapter/Convert/ToMidasCivil/Loads/FromBarVaryingDistributedLoad.cs Show resolved Hide resolved
MidasCivil_Adapter/CRUD/Create/Loads/BarVaryingDistributedLoads.cs Outdated Show resolved Hide resolved
MidasCivil_Adapter/CRUD/Create/Loads/BarVaryingDistributedLoads.cs Show resolved Hide resolved
@peterjamesnugent peterjamesnugent self-requested a review September 5, 2024 20:36
peterjamesnugent
peterjamesnugent approved these changes Sep 5, 2024
View reviewed changes
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.

I've tested this using the script, and added a few extra cases to test (e.g. multiple Bars with the same length it works. Able to load in to MidasCivil, and pull them back even having the same name.

Good first PR @EmmaSander!

@peterjamesnugent
Copy link
Member

@BHoMBot check required

@bhombot-ci BHoMBot-CI
Copy link

bhombot-ci bot commented Sep 5, 2024

@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

There are 24 requests in the queue ahead of you.

@peterjamesnugent
Copy link
Member

@BHoMBot check copyright-compliance

@bhombot-ci BHoMBot-CI
Copy link

bhombot-ci bot commented Sep 6, 2024

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

  • check copyright-compliance

There are 6 requests in the queue ahead of you.

@peterjamesnugent
Copy link
Member

@BHoMBot check versioning
@BHoMBot check installer

@bhombot-ci BHoMBot-CI
Copy link

bhombot-ci bot commented Sep 6, 2024

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

  • check versioning
  • check installer

There are 3 requests in the queue ahead of you.

@peterjamesnugent
Copy link
Member

@BHoMBot check installer

@bhombot-ci BHoMBot-CI
Copy link

bhombot-ci bot commented Sep 6, 2024

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

  • check installer

There are 2 requests in the queue ahead of you.

@peterjamesnugent
Copy link
Member

@BHoMBot check ready-to-merge

@bhombot-ci BHoMBot-CI
Copy link

bhombot-ci bot commented Sep 6, 2024

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

  • check ready-to-merge

@peterjamesnugent peterjamesnugent merged commit 9ada7b7 into develop Sep 6, 2024
10 checks passed
@peterjamesnugent peterjamesnugent deleted the MidasCivil_Toolkit-#270-SupportAbsoluteDistancesBarVaryingLoad branch September 6, 2024 16:36
@BHoMBot BHoMBot mentioned this pull request Sep 9, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@peterjamesnugent peterjamesnugent peterjamesnugent approved these changes

Assignees

@EmmaSander EmmaSander

Labels
good first issue Good for newcomers type:feature New capability or enhancement
Projects
None yet
Milestone
BHoM 7.3 β MVP
Development

Successfully merging this pull request may close these issues.

Add support for BarVaryingDistributedLoad that uses absolute distances
2 participants
@EmmaSander @peterjamesnugent