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

BHoM_Adapter: Distinct method in Replace causes assigned properties of coincident objects to be lost #126

Closed
peterjamesnugent opened this issue Aug 30, 2019 · 2 comments · Fixed by #355
Assignees
Labels
severity:medium Slows progress, but workaround is possible size:S Measured in minutes type:bug Error or unexpected behaviour

Comments

@peterjamesnugent
Copy link
Member

peterjamesnugent commented Aug 30, 2019

Description:

The Replace method in the BHoM_Adapter uses the Enumerable.Distinct method to cull duplicates using a comparer. If you push two lines that share a node, with one node being assigned a property, then that property will be lost when the lines are pushed.

How to replicate:

  1. Create two Point objects and a Line, that shares a Point with one of the existing Point
  2. Create a Bar using the defined line
  3. Create a Constraint6DOF
  4. Create two Node objects using the Point objects, and assign the support to the Node that is conicident with the Bar
  5. Create a Bar from the two Node objects
  6. Push both Bar objects using the same Push component

Expected behaviour:

When pushing the two Bar objects described above, the BHoM_Adapter should recognise that the Node objects have the same position but different properties and merge them.

This does raise the question about the scenario when the two Node objects have differing Constraint6DOF objects assigned.

Test file(s):

https://burohappold.sharepoint.com/:f:/s/BHoM/ErI9qiifV1NMhuiwH548i8UBamTci0e45pNA1XuJGPDOMA?e=qNFYUY

@peterjamesnugent peterjamesnugent added severity:medium Slows progress, but workaround is possible size:S Measured in minutes type:bug Error or unexpected behaviour labels Aug 30, 2019
@alelom
Copy link
Member

alelom commented Nov 13, 2019

When pushing the two Bar objects described above, the BHoM_Adapter should recognise that the Node objects have the same position but different properties and merge them.

The problem I see here is: how should they be merged? How can we define what merging means?
One example could be: take the Constraint values of the constrained Node and assign them to the "free" Node .

This could be formalised in defining what we mean for merging: merging two or more IBHoMObjects means to take all defined properties values of any of the objects involved, then and assign them to any of the objects involved that do not have them defined.

But as you have pointed out:

This does raise the question about the scenario when the two Node objects have differing Constraint6DOF objects assigned.

this in fact does not solve the issue when we have conflicting properties assigned.

We could define a "per-type" behaviour to solve this. For example, when trying to merge Constraint6DOF you could say that the "most restrictive" Constraint property values should be used.
This would need to be formalised together with the definition of merging.

@alelom alelom added type:feature New capability or enhancement and removed type:feature New capability or enhancement labels Nov 13, 2019
@IsakNaslundBh
Copy link
Contributor

This issue was highlighted again this morning in call with @Arne-Martensen .

Have a quite straightforward idea for how to fix it, and aiming to get a PR up today.

Had a quick call with @alelom regarding

This does raise the question about the scenario when the two Node objects have differing Constraint6DOF objects assigned.

and I think the only good way around it is to simply pick one (first) non-null constraint, and raise a note that there was a conflict. as it could be seen as user error.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
severity:medium Slows progress, but workaround is possible size:S Measured in minutes type:bug Error or unexpected behaviour
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants