Spatial_Engine: Migrate Methods from Common_Engine

[kThorsager]

Issues addressed by this PR

Closes #1470
Closes #1323
The methods which were in the Common_Engine and using IElement have been moved to the Spatial_Engine.

The methods in the Common_Engine now calls the new methods in the Spatial_Engine and are set as Deprecated

Test files

The Common_Engine methods

https://burohappold.sharepoint.com/:f:/s/BHoM/EkvCJbx7B_FMgCy56DDGfYEBxAYsn8yH10yFp5eqifX0JQ?e=R83f7s
https://burohappold.sharepoint.com/:u:/s/BHoM/Ec_mnMQcCUhGnsKUvm4QmQABsvvFgHg2CxXnVu98VQ5CIw?e=NPnW9I

The Spatial_Engine methods

https://burohappold.sharepoint.com/:f:/s/BHoM/EjAN0bkxublBirJzGJFTg8YBo5ZncAd1-AKSzDYJBPRoGQ?e=rKz1dB
https://burohappold.sharepoint.com/:u:/s/BHoM/EZpL86wOI89CgPhZhFXOROABs6rsYFynGUYKPn9Uchc4Yg?e=suXMbb

ElementCurves before/after

note that on this PR this will always work as the Common_Engine method calls the Spatial_Engine method
https://burohappold.sharepoint.com/:f:/s/BHoM/Epamw8TSDxxHjlHkazmmyHoBAjVUpiJT-6HYBUz_443CAQ?e=P6QeHE

Changelog

• IElementCurves is now usable with non-BHoMObjects

Additional comments

Considering whether to add the descriptions etc right away or handle in separate PR.
Will the compliance check allow that?
@FraserGreenroyd @al-fisher @pawelbaran

[pawelbaran]

Curious: shouldn't we set the two remaining arguments of Deprecated attributes (class and method name) for the sake of versioning? Migration of methods seems to me like a perfect case where this could be used.

[kThorsager]

I'm unaware if Versioning looks at the Deprecated attributes (other than to attempt to Version) and since neither the name nor the class have changed i figured it redundant.

and I'm also unsure of what exactly the Type newType refers to (output, input, which input?)

But if that's needed I'll change that.

Also unsure if the Versioning likes needing to first version from Geomerty.IElement => Dimensional.IElement and then Engine.Common => Engine.Spatial as the chaining of versioning is per quarter. (we could perhaps set the versioning to go directly from Geometry.IElement => Engine.Spatial but that would miss all scripts done in the meantime)

correct me if I'm wrong @adecler

[pawelbaran]

Right, both class and method names stay the same - thanks for answering, I will review the code today.

[IsakNaslundBh]

I'm unaware if Versioning looks at the Deprecated attributes (other than to attempt to Version) and since neither the name nor the class have changed i figured it redundant.

and I'm also unsure of what exactly the Type newType refers to (output, input, which input?)

But if that's needed I'll change that.

Also unsure if the Versioning likes needing to first version from Geomerty.IElement => Dimensional.IElement and then Engine.Common => Engine.Spatial as the chaining of versioning is per quarter. (we could perhaps set the versioning to go directly from Geometry.IElement => Engine.Spatial but that would miss all scripts done in the meantime)

correct me if I'm wrong @adecler

Maybe we can add upgrade for both? but might be to painful. This is, and Method upgrade in versioning toolkit from common to spatial for all methods moved, both using IElements from the new position, as well as the old, if that makes sense?

Code style changes have been addressed - see comment above

[kThorsager]

I got the Versioning to work for this if one removes the methods which I have now set a Deprecated, which should be fine, apart from the need to update every toolkit which uses the Common_Engine to Spatial_Engine this close to the beta release.

I know that at least Revit and ETABS are affected (easily amended), but have not done a closer query on it.

Not sure on how to get the versioning to work on it otherwise, so let me know if I should proceed?
@al-fisher @IsakNaslundBh @FraserGreenroyd @pawelbaran

[IsakNaslundBh]

Would not delete these methods now, as it requires more coordination across multiple repos as you say @kThorsager . Aim with this PR should be to make sure that all the new methods have been ported over correctly, and have tests to prove that is the case, and that all the to-be-deleted methods have been deprecated. Then in 3.2 we can calmly evaluate the impact of deleting them and make sure we raise PRs accordingly.

For the Versioning, as long as it is correct there is no real harm getting it merged in 3.1, as it only kicks in if it fails, but no real rush either (except for not keeping PRs open forever). Only important for it to be merged before we wipe all the common engine methods in 3.2.

Priority now should be to make sure we have sufficient testfiles for the migrated methods.

[kThorsager]

I have now added test-files for most of the methods, is it needed for every method, because in that case I'll need some help in both understanding what DistributeOutlines() does and how I'm supposed to call it in the canvas.

Otherwise this is ready

[kThorsager]

For the Versioning, as long as it is correct there is no real harm getting it merged in 3.1, as it only kicks in if it fails, but no real rush either (except for not keeping PRs open forever). Only important for it to be merged before we wipe all the common engine methods in 3.2.

Will give a problem when we switch over to Nugets in the Versioning_Toolkit and the BHoMUpgrader31 only can 'see' the code from the 3.1 beta release, and hence never proc if we make the changes which it would react to later.

[IsakNaslundBh]

For the Versioning, as long as it is correct there is no real harm getting it merged in 3.1, as it only kicks in if it fails, but no real rush either (except for not keeping PRs open forever). Only important for it to be merged before we wipe all the common engine methods in 3.2.

Will give a problem when we switch over to Nugets in the Versioning_Toolkit and the BHoMUpgrader31 only can 'see' the code from the 3.1 beta release, and hence never proc if we make the changes which it would react to later.

The methods to be upgraded are referenced by string, so should not be any issue there?

We will need to be able to reference things no longer existing in the versioning toolkit (by string) to be able to delete things at all.

[kThorsager]

For the Versioning, as long as it is correct there is no real harm getting it merged in 3.1, as it only kicks in if it fails, but no real rush either (except for not keeping PRs open forever). Only important for it to be merged before we wipe all the common engine methods in 3.2.

Will give a problem when we switch over to Nugets in the Versioning_Toolkit and the BHoMUpgrader31 only can 'see' the code from the 3.1 beta release, and hence never proc if we make the changes which it would react to later.

The methods to be upgraded are referenced by string, so should not be any issue there?

We will need to be able to reference things no longer existing in the versioning toolkit (by string) to be able to delete things at all.

Yes, you're right, I mixed up my thoughts
It's the methods which it upgrades into which must be in the same version,
sending it as the correct string happens on a UI level and should be fine

[IsakNaslundBh]

Some comments below about methods that should not be I methods. (know they were like that before, but might as well get it fixed now). Basically, I methods reserved to simple dispatchers (which I know you know 😄 )

The ported test-files all work fine. Some outstanding ones (did not exist before either, but think this could be a good opportunity to get them in):

Query

• Controlpoint (check external only functionality, basically, check that the boolean works, might require tweaks to the input data to check panels with openings)
• ElementCurves (especially check the "recursive" boolean)
• ElementVertices
• InternalOutlineCurves
• IsSelfIntersecting
• Normal
• OutlineCurve

Modify

• Translate

Then, after discussion, can leave DistributeOutlines out for now. It can be tested through the use of the Create.Panel method that takes a list of Outline curves and returns a list of Panels.

[pawelbaran]

Code LGTM, so I am happy to approve (@IsakNaslundBh's comments need to be resolved first, so this review is rather symbolic). One minor bug found in the test file, namely Verification cluster in Spatial_Engine/Length looks as below:

This, I guess, happened due to some versioning issue causing the wire to reset its origin?

Concerning DistributeOutlines, agreed with @IsakNaslundBh - this could be tested based on panel creators with various outline configurations.

[kThorsager]

Checked the test file @pawelbaran and it looks fine for me, are you using Rhino5?
The missing component is Grasshoppers subtraction which was updated between 5 and 6

[pawelbaran]

You are right @kThorsager, I was testing on Rhino5 and I got a subtraction error. Makes sense.

[kThorsager]

All test-files are now in the folder on SharePoint, so this is ready for review

[IsakNaslundBh]

All test-files are now in the folder on SharePoint, so this is ready for review

Well done @kThorsager . Will test things through!

[IsakNaslundBh]

/azp run BHoM_Engine.CheckInstaller

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[IsakNaslundBh]

Tested through, all testfiles run fine and code now looks fine!

[pawelbaran]

LGTM