Unified strategy for GUID, Hash, and Copy/DeepCopy

[alelom]

We need a unified strategy as title. In light of the meeting tomorrow morning, we should go through the following points:

• [x] Go through the definitions in https://github.com/BHoM/admin/issues/8. Make sure we're all on the same page. --> good conclusions and revision of some definitions
• [x] hash to be considered a more "base" property of BHoMObjects ? --> not needed, calculate upon necessity
• [x] Make sure the Modify Engine methods do not alter the BHoM_Guid. The object must not be re-instantiated, but always cloned.
  • Have a CI check on it?
  • Actually forcing this at a framework level, avoiding to rely on the implementer of the method? We could always force a clone when passing inputs to Modify Engine methods. --> Responsibility moved at the level of the UI in @adecler 's latest PR; @adecler to raise an issue to review where deepclone is used everywhere
• [x] BH.Engine.Base.Query.ShallowClone() vs BH.Engine.Base.Query.DeepClone() vs BH.oM.Base.GetShallowClone(). Agree whether it's best to keep all three of them or not. --> attribute on the Modify method to decide whether to do deepclone or shallowclone. By default, deepclone @adecler --> pull 200/500/1000 panels from revit --> check deepclone efficiency @IsakNaslundBh , @pawelbaran --> exclude the fragments/customdata from the deepclone in the UI level --> we can remove the BH.oM.Base.GetShallowClone() as the Query.ShallowClone() now relies on the Deepcloner extension
• [x] Move the Hash generation as up as possible - Base Engine would probably be problematic, but consider the Serialisation Engine, or another Engine called something more specific like "Hashing Engine". Diffing is too specific to the diffing. @al-fisher @alelom
• @pawelbaran note: SetProperty - hash would be preferred way

Instructions for the changes on Clone methods can be found on this comment below: https://github.com/BHoM/admin/issues/9#issuecomment-709032648

[adecler]

I'll create the list of methods calling DeepClone and ShallowClone soon.

[adecler]

First, here's the list of methods that contain Clone in their name and are called by at least on non-clone method:

BH.oM.Base.BHoMObject.GetShallowClone
BH.Engine.Base.Query.DeepClone
BH.oM.Base.IBHoMObject.GetShallowClone
BH.Engine.Geometry.Query.Clone
BH.Engine.Geometry.Query.IClone
BH.Engine.Base.Query.ShallowClone

The ShallowClone methods could probably do with a bit of clearing.

[adecler]

Here's the list of Modify methods that are calling one of the Clone methods above:

Full list

``` BH.Engine.Adapters.ETABS.Modify.SetDiaphragm(Panel panel, Diaphragm diaphragm) BH.Engine.Adapters.ETABS.Modify.SetPier(Panel panel, Pier pier) BH.Engine.Adapters.ETABS.Modify.SetShellType(ISurfaceProperty property, ShellType shellType) BH.Engine.Adapters.ETABS.Modify.SetSpandrel(Panel panel, Spandrel spandrel) BH.Engine.Adapters.Filing.Modify.AddContent(FSFile file, List

[adecler]

Here's the list of non-Modify methods calling one of the clone methods:

Full list

``` BH.Engine.Adapters.Revit.Query.Duplicate(IBHoMObject bHoMObject) BH.Engine.Adapters.SAP2000.Query.JoinRigidLink(List linkList) BH.Engine.Adapters.SAP2000.Query.SplitRigidLink(RigidLink link) BH.Engine.Architecture.Query.NewElement1D(Room room, ICurve curve) BH.Engine.Audience.Query.PlaceSeats(Row row, Double unitWidth, Cartesian world, and 1 more inputs) BH.Engine.CableNetDesign.Compute.Equilibrate(GravityCableNet cableNet, EquilibrateMethod method) BH.Engine.CableNetDesign.Compute.FixPlanRelaxationByMinHeight(GravityCableNet cableNet, EquilibrateMethod settings) BH.Engine.CableNetDesign.Compute.RelaxCables(GravityCableNet cableNet, EquilibrateMethod settings) BH.Engine.Diffing.Compute.DiffGenericObjects(IEnumerable

[adecler]

I have summarised the two lists above per namespace so we can tick them as they get fixed:

• [x] BH.Engine.Adapters.ETABS.Modify @IsakNaslundBh
• [x] BH.Engine.Adapters.GSA.Modify @IsakNaslundBh
• [ ] BH.Adapter.File @alelom
• [x] BH.Engine.Adapters.IES.Modify @FraserGreenroyd
• [x] BH.Engine.Adapters.Plaxis.Modify @IsakNaslundBh
• [x] BH.Engine.Adapters.Revit.Modify @pawelbaran
• [x] BH.Engine.Adapters.Revit.Query @pawelbaran
• [x] BH.Engine.Adapters.SAP2000.Query @IsakNaslundBh
• [x] BH.Engine.Adapters.TAS.Modify @FraserGreenroyd
• [ ] BH.Engine.Analytical.Modify @al-fisher
• [x] BH.Engine.Architecture.Modify @rolyhudson
• [x] BH.Engine.Architecture.Query @rolyhudson
• [x] BH.Engine.Architecture.Theatron.Create @rolyhudson
• [x] BH.Engine.Audience.Modify @rolyhudson
• [x] BH.Engine.Audience.Query @rolyhudson
• [x] BH.Engine.Base.Modify @alelom
• [ ] BH.Engine.CableNetDesign.Compute @IsakNaslundBh
• [ ] BH.Engine.CableNetDesign.Modify @IsakNaslundBh
• [x] BH.Engine.Diffing.Compute @alelom
• [x] BH.Engine.Diffing.Modify @alelom
• [x] BH.Engine.Diffing.Query @alelom
• [x] BH.Engine.Environment.Modify @FraserGreenroyd
• [x] BH.Engine.Environment.Query @FraserGreenroyd
• [x] BH.Engine.FormFinding.Modify @IsakNaslundBh
• [x] BH.Engine.Geometry.Compute @LMarkowski / @adecler
• [x] BH.Engine.Geometry.Convert @LMarkowski / @adecler
• [x] BH.Engine.Geometry.Create @LMarkowski / @adecler
• [x] BH.Engine.Geometry.Modify @LMarkowski / @adecler
• [x] BH.Engine.Geometry.Query @LMarkowski / @adecler
• [ ] BH.Engine.Graphics.Query @alelom
• [x] BH.Engine.Humans.ViewQuality.Create @rolyhudson
• [x] BH.Engine.MEP.Modify @FraserGreenroyd
• [x] BH.Engine.ModelLaundry.Modify @LMarkowski / @pawelbaran
• [x] BH.Engine.ModelLaundry.Query @LMarkowski / @pawelbaran
• [ ] BH.Engine.Physical.Create @al-fisher
• [ ] BH.Engine.Physical.Modify @al-fisher
• [x] BH.Engine.Reflection.Modify @adecler
• [x] BH.Engine.Representation.Modify @alelom
• [x] BH.Engine.Spatial.Query @LMarkowski
• [ ] BH.Engine.Structure.Bridges.Modify @peterjamesnugent
• [ ] BH.Engine.Structure.Compute @IsakNaslundBh
• [ ] BH.Engine.Structure.Create @IsakNaslundBh
• [ ] BH.Engine.Structure.Elements.Modify @IsakNaslundBh
• [ ] BH.Engine.Structure.Generative.Create @IsakNaslundBh
• [ ] BH.Engine.Structure.Modify @IsakNaslundBh
• [ ] BH.Engine.Structure.Query @IsakNaslundBh
• [ ] BH.Engine.Structure.Trusses.Compute @IsakNaslundBh

[adecler]

Just to confirm: the DeepClone and ShallowClone methods are returning an object with the same Guid already:

Agreed we should get rid of the other clone methods (including the geometry ones)

[adecler]

GetShallowClone from the oM and GeometryClone methods will be removed completely. So the onyl two remaining Clone methods are Engine.Base.Query.DeepClone() and Engine.Base.Query.ShallowClone()

Instructions:

• Replace calls to Geometry.Clone and Geometry.IClone to calls to Engine.Base.DeepClone(). Feel free to use Engine.Base.ShallowClone() where you feel appropriate.
• Replace calls to GetShallowClone with calls to Engine.Base.ShallowClone()
• Remove the as X conversion after the cloning since casting is not necessary anymore

See the PR on the Geometry engine for an example: https://github.com/BHoM/BHoM_Engine/pull/2070

To be actioned as soon as convenient. Can be done separately but need to be done by the end of sprint 4.

Once all the calls to Clone methods have been cleaned, I will delete the four clone methods mentioned above.

@al-fisher , @adecler , @alelom , @IsakNaslundBh , @FraserGreenroyd , @pawelbaran , @LMarkowski , @peterjamesnugent , @rolyhudson , FYI

[IsakNaslundBh]

Remove any reference to any clone method in the Modify namespace. If there is a good reason to keep it, please mention it here

Would be a bit careful with this, especially for any modify methods that are related to the IElement workflows. A lot of the methods that target IElement currently relies on the objects being cloned at modify. For example splitting something assumes that you can just call SetGeometry() twice with different geometries and get two of objects back that have different geometries but all other properties maintained exactly the same.

Can definitely get behind getting rid of GetShallowClone() by the end of sprint 4, but getting rid of cloning in every single modify method I would be a bit more careful with for the reasons stated above. For each method we basically need to check the implications it has on methods calling it, which can be non-trivial as the methods calling it might be in another toolkit, and even more so, called via some interface via RunExtensionMethod.

[al-fisher]

Understood - we need to understand the scale of the work. Perhaps worth a session specifically on this again next week after individuals have looked a specific cases? Can we link to some specific methods and how they might need to be tackled? I know @adecler was going to look at some of the reflection and geometry cases over the next few days to look a concrete issues for other also to watch out for.

As you say - priority, as I see it, is to remove all use of BH.oM.Base.BHoMObject.GetShallowClone. If that means replacing with BH.Engine.Base.Query.ShallowClone that is fine. Removal of BHoMObject.GetShallowClone is of course important in the needed reliance of GUID and use of Hash.

[adecler]

I think it is important to stress the fact that Modify methods are now ment to always modify the input object. This will be reinforced by later making the return type of ALL Modify method void (see https://github.com/BHoM/admin/issues/11).

The idea behind this is simple:

• Objects in C# are always mutable (this is how C# wants to work by default)
• Objects in UI scripts are always immutable (this is how UIs like Grasshopper want to work by default)

The BHoM_UI is already taking care of handling the discrepancy between the two realms by handling the cloning when appropriate.

This means that

• Modify methods should remove the step of first cloning the input object
• Modify methods that were creating a new object should modify the input object instead
• Modify methods that were returning a new type of object are not compliant and should move to either Query or Compute as they are already breaking the compliance rules.

This is why I really like @alelom 's idea of having the Modify methods returning null because it leaves you no choice but to be compliant on top of being far more intuitive when deciding how to use those methods 😄 . We will, however, enforce the output to be void on a second phase to avoid too much mess.

I hope that makes the intent a bit more clear and helps you when you take on the tasks above. I will try to share my experience of fixing the Modify methods for the Geometry engine as much as possible but happy to have other meeting for those that still find this change confusing.

[IsakNaslundBh]

I think it is important to stress the fact that Modify methods are now ment to always modify the input object. This will be reinforced by later making the return type of ALL Modify method void (see #11).

Yeah, understand this ofc. All I am saying is that we can not simply look at the methods in isolation. You need to find all calls in the code, in all toolkits that can reach that method and evaluate if the implicit cloning mechanism that we have up until this point enforce was used by any of the methods.

I do not know how widespread this is, but as stated in the previous comment, I know it happens in quite a few cases for the IElements

[adecler]

Yeah, understand this ofc. All I am saying is that we can not simply look at the methods in isolation. You need to find all calls in the code, in all toolkits that can reach that method and evaluate if the implicit cloning mechanism that we have up until this point enforce was used by any of the methods.

Exactly, that what I meant by

Check all methods calling a method with a deleted call to Clone to make sure they don't rely on the object being cloned.

Fun, isn't it ? 😉

What I recommend is to first fix all Modify methods in isolation and then check all calling methods. Not only will you naturally solve the problem for Modify methods calling other Modify methods but it will also help you stay focus on the first part of the task without running around in the code too much.

[adecler]

FYI, I have started a discussion around the Modify methods with an output type different than their first input type here: https://github.com/BHoM/BHoM/discussions/1031

[adecler]

All,

I have updated the instructions for correcting the calls to Clone methods (https://github.com/BHoM/admin/issues/9#issuecomment-709032648) base on our recent discussions. The purpose is now really focused on two things:

• Guarantee the BHoMGuid is kept identical after cloning (GetShallowClone was allowing for it to be replaced)
• Get rid of the four Cloning methods that we agreed were redundant.

This means that making the required changes has now become extremely simple and fast to execute 😄 🎉

[FraserGreenroyd]

I believe my parts of this are done so I'm unassigning myself - please shout if I missed anything!

[adecler]

@alelom , @al-fisher , @IsakNaslundBh , gentle reminder that this is still pending.

[al-fisher]

Thanks @adecler - PRs raised for Analytical and Physical

[al-fisher]

@alelom - I have just raised a new issue https://github.com/BHoM/BHoM/issues/1259 - which we should now aim to close out.

A quick Ctrl-F of "GetShallowClone" in particular critical for above, but also "IClone" - shows a few GetShallowClones are still there/crept back in over the last 6 months!

As per the new issue on the BHoMObject just raised - worth us crowding round and quickly putting this to bed - with priority cleaning up the Base.BHoMObject 🌟 @adecler @IsakNaslundBh FYI

[alelom]

All remaining calls to the oM's GetShallowClone() method have been now targeted by PRs to remove them. I've also removed calls to IClone where present (Analytical_Engine, Structure_Engine):

• [x] https://github.com/BHoM/GSA_Toolkit/pull/247
• [x] https://github.com/BHoM/File_Toolkit/pull/120
• [x] https://github.com/BHoM/Robot_Toolkit/pull/464
• [x] https://github.com/BHoM/Test_Toolkit/pull/351
• [x] https://github.com/BHoM/Revit_Toolkit/pull/1057
• [x] https://github.com/BHoM/BHoM_UI/pull/398
• [x] https://github.com/BHoM/BHoM_Engine/pull/2550
• [x] https://github.com/BHoM/BHoM_Engine/pull/2552
• [x] https://github.com/BHoM/BHoM_Engine/pull/2554
• [x] https://github.com/BHoM/BHoM_Engine/pull/2556
• [x] https://github.com/BHoM/BHoM_Engine/pull/2558
• [x] https://github.com/BHoM/BHoM_Engine/pull/2562
• [x] https://github.com/BHoM/BHoM_Engine/pull/2563
• [x] https://github.com/BHoM/BHoM_Engine/pull/2564
• [x] https://github.com/BHoM/BHoM_Engine/pull/2566

Implementations of GetShallowClone to be removed after it is removed from the IBHoMObject interface:

• [x] https://github.com/BHoM/XML_Toolkit/issues/532 - @FraserGreenroyd

[adecler]

@alelom , you can find all the methods calling GetShallowClone using something like this:

[FraserGreenroyd]

@alelom to capture PR comments - MEP Engine and Facade Engine PRs do not match the files they claim to change, MEP Engine PR was dealing with Physical Engine files, and Facade Engine PR was dealing with Structure Engine.

Those two engines might need reinvestigating?

[FraserGreenroyd]

Ok all sorted now by the looks of it, thanks @alelom

[FraserGreenroyd]

@alelom I've merged all your PRs for you. The one on XML needs to be done in coordination with the one on BHoM, same branch, etc. - lemme know when you want to do that.

[alelom]

Wonderful! Thanks @FraserGreenroyd

[alelom]

The method GetShallowClone has now been removed from the base BHoMObject: https://github.com/BHoM/BHoM/pull/1265.

[alelom]

Final action on the unified strategy for the Cloning is to get rid of the last remaining Clone method: BH.Engine.Geometry.Query.Clone and BH.Engine.Geometry.Query.IClone.

All calls those two methods will be replaced with calls to Engine.Base.DeepClone(). This is actioned here: https://github.com/BHoM/BHoM_Engine/pull/2585

[FraserGreenroyd]

Ok we've merged that today as well now @alelom - does that conclude this issue?

[alelom]

With https://github.com/BHoM/BHoM/pull/1265 and https://github.com/BHoM/BHoM_Engine/pull/2585 merged we now are left with a unified strategy for cloning. In other words, now BHoM only defines two cloning methods: BH.Engine.Base.DeepClone() and BH.Engine.Base.ShallowClone() – both relying on the open source library DeepCloner.

This removes any ambiguity on how to clone objects in BHoM. Monitoring for performance should continue as well as logging for features we may need from the DeepCloner library.

[al-fisher]

Fantastic! This is great @alelom @FraserGreenroyd 👍