Physical_Engine - Add create methods for Wall, Roof and Floor

[ZiolkowskiJakub]

NOTE: Depends on

N/A

Issues addressed by this PR

Closes #1125

Test files

Changelog

• Create cs files added to the solution: Wall, Floor, Roof

Additional comments

N/A

[FraserGreenroyd]

Have put some comments in the code on specifics that I'd like to see changed before approving (you can ignore the Oxford Comma if you really want but the rest are more necessary 😉 )

However, we seem to have 2 of every method with similarish inputs? This could cause confusion on the UI side (GH/Dynamo), when users go through the drop-down menus and see two options for creating a given Physical element.

When we refactored the Environment Engine, we opted to do away with most of our create methods and go for a single method which had defaults on every input. This allowed us to have a single option in the UIs, and still pick/choose the create method within the toolkits by naming the specific input.

Of course, from a development point of view, polymorphism is always going to be my favourite. But from a practical point of view, too many options in the UIs adds confusion to the novice user.

That said, I'm not going to hold up this PR on this particular point as this might need further discussion (there are pros and cons to both sides, as with most things) so for now, just the comments in the code. But we might want to look at the single method with named inputs as we did for Environments in the future.

Hope that makes sense? 😄

[IsakNaslundBh]

Would add as a comment here as well, please have a look in the Surface.cs file in the create folder. It already contains a few create methods for the surfaces, important that those not overlap.

Might have been the wrong choice by me to group them together though. Whatever we decide, it needs to be consistent, so either the previous ones needs to be moved to the new files or vice versa.

I would agree with @FraserGreenroyd at keeping create methods to a minimum, but still think there are some cases that will be useful for these surfaces, such as creating a wall from a base curve and a height etc. But again, as I think both of us have added this method, it needs to be consolidated.

[ZiolkowskiJakub]

I would agree with @FraserGreenroyd at keeping create methods to a minimum, but still think there are some cases that will be useful for these surfaces, such as creating a wall from a base curve and a height etc. But again, as I think both of us have added this method, it needs to be consolidated.

Hi @IsakNaslundBh,

This is why I initially create these constructors to Revit_Toolkit... Maybe we do not need it globally. I believe these are quite handy for Revit purposes.

The reason I moved it here was the issue you pointed out:
BHoM/Revit_Toolkit#269

Hi @FraserGreenroyd

However, we seem to have 2 of every method with similarish inputs? This could cause confusion on the UI side (GH/Dynamo), when users go through the drop-down menus and see two options for creating a given Physical element.

Yes.. there reason why I separated these is Dynamo have some issue with IEnumerable set to null as default

[IsakNaslundBh]

@ZiolkowskiJakub Maybe I was not clear enough 😃 , I meant that I agree with having some extra constructors, but that we just should be a bit careful not to overflow.
From the user side it does not really matter if they are in base BHoM engine or in the Revit toolkit, they will show up in the menus nonetheless, but again, a couple simplying the process I think is a good thing!

My main point was that there already are some create methods in the physical_Engine here: https://github.com/BHoM/BHoM_Engine/blob/master/Physical_Engine/Create/Surface.cs

for the objects you are adding constructors to. Would you be able to either move those into your new files OR move the ones you have added into the old files? Think option 1 is better, moving the old ones to the new files.

Also when doing this, could you double check that we do not have any overlaps between the methods implemented?

[ZiolkowskiJakub]

Hi @IsakNaslundBh

Thank you for clarification. Sorry but "Surface" would not be the place where I look for Wall, Floor or Roof create methods.

Could we split and name it after BHoM object type name?

[IsakNaslundBh]

@ZiolkowskiJakub Yes, that is what I meant :)

Could you please move the create methods out from the surface file to the files you have created instead, then we can delete the file I just linked to.

[FraserGreenroyd]

Hi @ZiolkowskiJakub sorry for the delay in this, some minor changes requested - see code comments.

[epignatelli]

@ZiolkowskiJakub to have a look at the issues about descriptions today in order to be closed.
@IsakNaslundBh @epignatelli to help review

[FraserGreenroyd]

@ZiolkowskiJakub did you manage to take a look on Friday? Would be good to either get this closed out for 2.4MVP or park it for 2.5.

[ZiolkowskiJakub]

Hi @FraserGreenroyd,

It looks good to me. However I would also overload all these methods with Tolerance parameter as I mentioned last time.

[ZiolkowskiJakub]

I would also overload all these methods with Tolerance parameter as I mentioned last time.

This is for 2.5MVP

[FraserGreenroyd]

Hi @ZiolkowskiJakub do we have an eta on this PR? 😄 there are some outstanding comments from my review - would be good to get this planned at some point? 😄

[FraserGreenroyd]

Thanks @ZiolkowskiJakub for the updates, this now LGTM in reference to my previous outstanding comments.

@IsakNaslundBh or @al-fisher do either of you want to cast your eye over this before merging? I'll give this a 24 hour cool-down on my review for further comments before merging 😄

[FraserGreenroyd]

24 hour cool down has definitely passed now - opps! Merging on the basis of the above 😄