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

Structure_oM: adding a new DistanceIsParametric bool property to BarPointLoad #1485

Closed
wants to merge 1 commit into from

Conversation

alelom
Copy link
Member

@alelom alelom commented Feb 28, 2023

NOTE: Depends on

Issues addressed by this PR

Closes #1427

Adds a new DistanceIsParametric boolean to BarPointLoad to clarify whether DistanceFromA property is parametric or absolute. Adjusts the description to match.

Test files

No workflows support this yet. Done in support of BHoM/Karamba3D_Toolkit#4 (comment).

Changelog

Additional comments

@alelom alelom added size:XS Measured in seconds type:feature New capability or enhancement labels Feb 28, 2023
@alelom alelom self-assigned this Feb 28, 2023
@alelom alelom requested a review from rwemay as a code owner February 28, 2023 14:27
@IsakNaslundBh IsakNaslundBh added the status:do-not-merge For instance, test PR, for discussion, or dependant PRs not ready for merge label Feb 28, 2023
@IsakNaslundBh
Copy link
Contributor

Putting a do-not-merge tag on this, as I do not think we should merge this until all adapters currently supporting this load type has been adjusted to either:

  1. Support this change, and be able to handle it
  2. Raise errors telling the user it is not yet supported.

Without this, this can make for a situation where you create a load in BHoM that is expected as one thing, but comes with different meaning in the software it is pushed to.

Also, need to align methods like the Visualize method in the Structure_Engine to accurately display the loads.

@FraserGreenroyd
Copy link
Contributor

@alelom @IsakNaslundBh is there an update on this PR and the work needed to close it out? Keen to ensure we have adequate resource to complete as necessary.

@IsakNaslundBh
Copy link
Contributor

@FraserGreenroyd There is quite a bit of work still outstanding, namely, some updates to engine methods, such as the visualisation, and above that, updates to all toolkits to make sure they can handle this new property.

Think we also should update the BarVaryingDistributedLoad with the same boolean once we decide to go ahead with this PR.

@FraserGreenroyd
Copy link
Contributor

@IsakNaslundBh @alelom any further update to this in the last month? Conscious of your impending (planned and short term!) departure @IsakNaslundBh and wouldn't want anything hanging unnecessarily? 😄

@alelom
Copy link
Member Author

alelom commented May 17, 2023

After discussion with @IsakNaslundBh , it appears that the work for this PR is quite extensive as it requires a wider refactoring in several places. If we add this feature here, we also need to align BarVaryingDistributedLoads, the Create methods in Structure, the Visualisation methods, and all toolkits will need to be able to correctly push and check for this type of load.

Because the initial reason behind this was the development around Karamba, which has now reached a stable point, there is no priority to push this further. With @IsakNaslundBh we've both agreed that this is a feature we'd need to introduce, but a wider workshop/refactoring effort needs to be planned in order for it to be carried out.

@alelom alelom closed this May 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size:XS Measured in seconds status:do-not-merge For instance, test PR, for discussion, or dependant PRs not ready for merge type:feature New capability or enhancement
Projects
None yet
3 participants