Common_Geometry_Engine: Add Distance methods for IElementsXD

[kThorsager]

Issues addressed by this PR

Closes #1313
Closes #1323

Test files

Changelog

• Adds distance methods for IElementsXD
• Migrates IElementsXD methods from Common_Engine to Geometry_Engine

Additional comments

This messes with both Common_Engine and Geometry_Engine

Took some liberties to get Bounds.cs to work, wouldn't mind a extra check on that.

Would need some help on coordination both of when to be able to merge it and checking depending repos, (not sure of how much to trust the CI) @rwemay @pawelbaran

Small file change in Environment_Engine as well

[kThorsager]

After taking a bit of a closer look at why Bounds was being a bother I noticed that Tex things like ICurve is both IGeometry and IElement1D which causes problems when mixing interface methods since it can't know which to call.

So can't say I have thought trough this thoroughly yet but could we add something like ISpatial which encompasses both with accompanying methods Geometry and SetGeometry which would start of and end every geometrical operation. (I think they for the most part exist already, in wich case it would mostly be to reformat the interface methods)
@pawelbaran

[pawelbaran]

@kThorsager could you explain the issue you mention above in more detail? A solid example/snippet of where does the code break? Intuitively I believe we should be OK without adding any new interfaces.

[kThorsager]

@pawelbaran The lower two methods used to be IBounds but things like ICurve are both IGeometry and IElement so that broke, which is why I added the top method

[pawelbaran]

Your proposal does not look good, but might be the only way to go - I would only recommend making Bounds(IGeometry) and Bounds(IElement) private as they are not compliant (their name should be IBounds).

We can flag it up and discuss in a wider group once the PR is ready.

[kThorsager]

I agree it does look quite horrendous.

But a note is that I was thinking of holding of on this PR until #1319 and/or #1314 is merged since they all mess with the .csproj.

But if we want this merged soon I could probably make it ready today, but I'm expecting that there might be a lot of dependent repos that'll need updating, which would slow things down.

[pawelbaran]

Yeah I would definitely not rush with that one as long as there is no legitimate urgency. @rwemay @al-fisher

[adecler]

Can you explain the point of having IBounds(this IObject element) ? I cannot think of a case where objects would be handled not as a list of IGeometry or a list of IElement but as a list of both.
This also feels very wrong to explicitely provide a method for all IObject while you are actually only covering 2 specific types internally.

[kThorsager]

ICurves are both IElement and IGeometry, hence a list of ICurves are a list of both.

This is some initial work before I realized the scope of the problem and is really not the solution I would go for.

[kThorsager]

I have tried to take a closer look at the issues with keeping the IElement and IGeometry methods in the same Engine and while it should work fine for some objects, things like every ICurve and Point are both, which makes it problematic to have both one and two interface method, as it's either;

• A single Interface method, which must encompass things which are, only IElement, only IGeometry, and both IElement and IGeometry. (Would now need to apply to IObject, but that would touch a lot of other things as well)
• Two Interface methods, one for IElement and one for IGeometry. But that will break for ICurve and Point as they are both.

So some options to resolve this as I see it would be:

• Keep them in different engines, i.e. let the user explicitly state which to use
• Make all IGeometry inherit from IElement, (at least the non-abstract ones, for example not TransformMatrix)
• Create a new Interface encompassing both IGeometry and IElement and not the rest of the BHoM
• Separation, where ICurve and Point is only IGeometry
• Use IObject and throw errors for everything that isn't a IGeometry or IElement

But in general, the way things inherits in the Geometry_oM is a bit back and forth between IGeometry and IElement and a second look at all of that might be good. @pawelbaran @rwemay @al-fisher
And would in general like to have some direction on how we want to fix this before I do it

[pawelbaran]

Just thinking: maybe it would be a bit hardcore, but what if we ditched IElement and used IGeometry as a parent interface for IElement0D, IElement1D and IElement2D? Since all IElements are meant to sit in Geometry_oM, we should be able to say they all inherit from IGeometry?

Then, we could ditch all interface methods working on IElement and implement ones working on IGeometry instead if they do not exist yet. I have just managed to brute-force refactor and compile it on my machine - looks like it should work? @al-fisher @rwemay @IsakNaslundBh @FraserGreenroyd

[FraserGreenroyd]

Just thinking: maybe it would be a bit hardcore, but what if we ditched IElement and used IGeometry as a parent interface for IElement0D, IElement1D and IElement2D? Since all IElements are meant to sit in Geometry_oM, we should be able to say they all inherit from IGeometry?

Then, we could ditch all interface methods working on IElement and implement ones working on IGeometry instead if they do not exist yet. I have just managed to brute-force refactor and compile it on my machine - looks like it should work? @al-fisher @rwemay @IsakNaslundBh @FraserGreenroyd

I thought IElement was used on analytical objects like panel? Or was that a different interface?

[pawelbaran]

That was this one. It is meant to identify the analytical elements that have geometrical representation of certain type, so maybe it could just be replaced with IGeometry? Especially taken into account that it sits in the Geometry_oM and has been extended to encompass ICurve and Point too...

Alternatively, I would opt for excluding ICurve and Point from IElements.

[FraserGreenroyd]

That was this one. It is meant to identify the analytical elements that have geometrical representation of certain type, so maybe it could just be replaced with IGeometry? Especially taken into account that it sits in the Geometry_oM and has been extended to encompass ICurve and Point too...

Alternatively, I would opt for excluding ICurve and Point from IElements.

I would prefer that last option myself, I don't personally feel that an Analytical Panel should be tagged as IGeometry cause it is more than just Geometry and might confuse people?

[pawelbaran]

I would prefer that last option myself, I don't personally feel that an Analytical Panel should be tagged as IGeometry cause it is more than just Geometry and might confuse people?

That is why it inherits directly from IElement2D. Inheritance from IGeometry would be sort of hidden higher up the chain. And for the sake of discussion: if the analytical panel has geometrical representation, doesn't it mean it inherits some part of its characteristic from IGeometry?

Maybe I am messing it up too much, but we could also keep IElement interface, but just make it inherit from IGeometry?

[IsakNaslundBh]

I would prefer that last option myself, I don't personally feel that an Analytical Panel should be tagged as IGeometry cause it is more than just Geometry and might confuse people?

That is why it inherits directly from IElement2D. Inheritance from IGeometry would be sort of hidden higher up the chain. And for the sake of discussion: if the analytical panel has geometrical representation, doesn't it mean it inherits some part of its characteristic from IGeometry?

Maybe I am messing it up too much, but we could also keep IElement interface, but just make it inherit from IGeometry?

Would not say a bar or panel or node is geometry. They have geometry, so personally would not want to go down that route of making all IElements implement IGeometry. Also, does not really matter if it is "hidden" in the inheritance chain, it would still be there.

In my mind I still think we need both interfaces, and they have clear defined roles:

IGeometry - for any obejct that is just pure geometry, nothing more nothing less
IElement - for something that can get and set geometry of different types.

As for curve/point, we would still need them to share an interface to be able to use a curve/bar interchangeably, right? So personally would not remove that implementation.

Might be missing the bigger picture here, but why is it a problem? Has been working well so far?

EDIT sorry, reading through @kThorsager s comments above a bit closer now. Would opt for a solution that does not drastically changes the object heirachy for this personally, unless we have to still. If it means separate engines I would prefer that over messing with the interfaces.

[pawelbaran]

Hmm, looking at the comments above I have an impression that we might need to come back to the core question if IElements should belong to the Geometry universe or not 🤔

[IsakNaslundBh]

Hmm, looking at the comments above I have an impression that we might need to come back to the core question if IElements should belong to the Geometry universe or not 🤔

Yeah, agree with you. IMO, at least the methods feels a bit misplaced in the Geometry_Engine, and then the same sort of must hold true for the oM.

[kThorsager]

@al-fisher @rwemay @IsakNaslundBh @FraserGreenroyd @pawelbaran @adecler
This discussion feels very much still up in the air and something which would need a proper answer.

Some general solution options are outlined further up with a graph of the object hierarchy.

There is also another point being brought up by @pawelbaran on whether IElement belongs in the Geometry_oM.

It would be good to get some answers so that this PR can finally get some work done on it.

[IsakNaslundBh]

@kThorsager I still see no problem with the object hierarchy.

IMO, but happy for others to chip in, I would keep those methods out of the Geometry_Engine and keep the Geometry_Engine for just simple plain geometry. If that mean finding another place for the interfaces I would rather do that then having this kind of overlap happening. Could see them sitting in the main BHoM project for example.

A question back to you, what triggered than mass moving of all of those methods in the first place? I see no record of it in any of the issues, but know getting rid of Common_Engine has been discussed before. IMO a strongly question that now, given the problems seen in this PR.

[al-fisher]

Reading this thread and trying to get my head into it.
Agree with @pawelbaran @IsakNaslundBh above.

IMO, at least the methods feels a bit misplaced in the Geometry_Engine, and then the same sort of must hold true for the oM.

And I agree with @IsakNaslundBh comment above, that the concept of an ElementXD is something that owns geometry.

The Analytical_oM is surely the right location for IElementXDs ? - not looking back at the full history ... but Common and Geometry surely pre-dated Analytical and Physical. which would explain the reason we have them in Geometry now?
Or would that cause problems between Analytical and Physical themselves? Need to check

Before we make a decision will be good to add more to the diagram above you have produced @kThorsager - to clearly map out and document the wider relationships and reasoning also. Can add to the https://github.com/BHoM/documentation/wiki/Geometry wiki pages to help explanation going forward.
Keen to help with that!
Very useful for us to document to help this conversation (and our future selves).

[al-fisher]

Ah thanks @kThorsager for link above 👍

Then, as above and last comments on that thread - do you want to have a stab at mapping out the relationships and diagrams further? If you share a link to your diagram above - super happy to help develop it further. With @pawelbaran and others to help review I am sure.

Does that sound like a good idea to help define clear next steps?

[kThorsager]

Yes, I'm for having an other stab at it, I was however really not expecting to need the diagram file right now and it is as such saved locally in London, where I'm not until next week...
But am I right in thinking that the diagram should mainly outline objects which geometrical operations can be preformed on, or should It be something more generall for the BHoM?

[al-fisher]

Perfect - would not worry about a perfect diagram now - (just take the image above and sketch over and add to will be fine we can polish later when happy)

But am I right in thinking that the diagram should mainly outline objects which geometrical operations can be preformed on?

Yep. Exactly. Diagramming out the object relationships and then annotating with their wider dependancies, functionality and responsibilities. This will help us to see at a high level what potential conflicts there might be if we want to plan any further improvements or changes.
@FraserGreenroyd @IsakNaslundBh can you share the diagrams and work you did from the discipline object alignment? Will be useful for @kThorsager to see.

And then in fact will be good to find a home for all of these high level object diagrams on the wiki as documentation. @FraserGreenroyd good support to the API generation we have discussed for this milestone.

[FraserGreenroyd]

https://github.com/BHoM/BHoM/wiki/Environment_oM---Geometrical - associated links here have the Environment diagrams we did, I can probably dig out the original with both disciplines from somewhere tomorrow

[adecler]

First time I am seeing this so it might just be me being outside the loop on this one but I cannot see a good reason to move methods on IElement to the Geometry engine. What was the incentive to do that in the first place?

[pawelbaran]

As far as I remember, the whole idea originates from the concept of ditching Common namespace.

Btw, could be worth to resolve #605 in this thread as well. @rwemay

[al-fisher]

Btw, could be worth to resolve #605 in this thread as well. @rwemay

Yes!! 👍
@kThorsager important we include both Analytical and Physical in the mapping exercise above.

[kThorsager]

Figuered that this excersise migth work out better with some computation, so I created a script which mapps out the relations between objects in some given oMs like this:

The component also finds which types each type is using, but am not showing that in this graph.

I do however think that there might be better software's for displaying data graph parametrically than rhino, especially if we would like to place something interactive in the wiki.

Is this along the line of what you wanted @al-fisher ?

Branch: BHoM_Engine-DisplayoMHeirarcy (placed it in the data engine in lack of better place)
GH file

[al-fisher]

This is cool - great stuff @kThorsager - super useful.
Will be great to now sketch the idealised/target relationships and overlay the two (target with actual) so we can see what inconsistencies may be in place now and indeed plan next steps.

[kThorsager]

The IElement methods are no longer planed to be moved to the Geometry_Engine.
That will introduce enough change to this PR that it will be easier to start form scratch.
Will hence close this and resume in other PRs after the IElement migration to Dimensional_oM