Unified strategy for GUID, Hash, and Copy/DeepCopy #9
Comments
alelom
added
type:feature
|
I'll create the list of methods calling DeepClone and ShallowClone soon. |
|
First, here's the list of methods that contain The |
|
Here's the list of Full list
|
|
Here's the list of non-Modify methods calling one of the clone methods: Full list
|
|
I have summarised the two lists above per namespace so we can tick them as they get fixed:
|
|
Just to confirm: the
Agreed we should get rid of the other clone methods (including the geometry ones) |
|
GetShallowClone from the oM and GeometryClone methods will be removed completely. So the onyl two remaining Clone methods are Instructions:
See the PR on the Geometry engine for an example: BHoM/BHoM_Engine#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 |
Would be a bit careful with this, especially for any modify methods that are related to the 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 |
|
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? As you say - priority, as I see it, is to remove all use of |
|
I think it is important to stress the fact that The idea behind this is simple:
The BHoM_UI is already taking care of handling the discrepancy between the two realms by handling the cloning when appropriate. This means that
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. |
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 |
Exactly, that what I meant by
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. |
|
FYI, I have started a discussion around the Modify methods with an output type different than their first input type here: BHoM/BHoM#1031 |
|
All, I have updated the instructions for correcting the calls to Clone methods (#9 (comment)) base on our recent discussions.
This means that making the required changes has now become extremely simple and fast to execute 😄 🎉 |
|
@alelom , you can find all the methods calling
|
|
@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? |
|
Ok all sorted now by the looks of it, thanks @alelom |
|
@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. |
|
Wonderful! Thanks @FraserGreenroyd |
|
The method GetShallowClone has now been removed from the base BHoMObject: BHoM/BHoM#1265. |
|
Final action on the unified strategy for the Cloning is to get rid of the last remaining Clone method: All calls those two methods will be replaced with calls to |
|
Ok we've merged that today as well now @alelom - does that conclude this issue? |
|
With BHoM/BHoM#1265 and BHoM/BHoM_Engine#2585 merged we now are left with a unified strategy for cloning. In other words, now BHoM only defines two cloning methods: 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. |
|
Fantastic! This is great @alelom @FraserGreenroyd 👍 |
We need a unified strategy as title. In light of the meeting tomorrow morning, we should go through the following points:
ModifyEngine methods do not alter the BHoM_Guid. The object must not be re-instantiated, but always cloned.--> 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
BH.Engine.Base.Query.ShallowClone()vsBH.Engine.Base.Query.DeepClone()vsBH.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 theQuery.ShallowClone()now relies on the Deepcloner extensionInstructions for the changes on Clone methods can be found on this comment below: #9 (comment)
The text was updated successfully, but these errors were encountered: