Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Common_Engine: Centralise methods for IElementXD's #1262

Open
rwemay opened this issue Oct 17, 2019 · 4 comments
Open

Common_Engine: Centralise methods for IElementXD's #1262

rwemay opened this issue Oct 17, 2019 · 4 comments
Assignees
Labels
type:question Ask for further details or start conversation

Comments

@rwemay
Copy link
Member

rwemay commented Oct 17, 2019

Description:

We have several duplicate methods in different discipline/repo's for creating elements (e.g. IElement2D panels). Would be great to centralise some of these in the Engine (is Common the right place - or are we moving IElement methods?). As a starter for 10:

IElement2D

  • Rectangular - creates a 4 sided panel, in a (defaulted) cartesian coordinate system;
  • Vertical - creates a panel in cartesian Z;
  • VerticalRectangular - creates a 4 sided panel in cartesian Z;
  • AddOpening - I initially thought we could remove this from Environment object and just use 'set property' but I suspect this has been added so you can append openings to a panel (instead of replacing the list)? It made me think we might want some form of 'append' alongside SetProperty for this type of case. Could be tricky with nested lists though.

Thoughts/comments appreciated!

image

@rwemay rwemay added the type:question Ask for further details or start conversation label Oct 17, 2019
@rwemay rwemay changed the title Common_Engine: Centralise methods for creating IElementXD's Common_Engine: Centralise methods for IElementXD's Oct 17, 2019
@pawelbaran
Copy link
Member

@rwemay I am not sure if I understand your idea. IElements are interfaces, so cannot be instantiated as such - what should be created after calling the centralised method you are requesting?

An enum argument corresponding with the discipline could be a nice way to feed the info about expected object type, but this would need to be added in the BHoM first. I imagine we have reasons not to have it until now, @al-fisher @IsakNaslundBh @FraserGreenroyd?

By the way, wasn't Common meant to be removed/replaced (vide discussion #1204)?

@IsakNaslundBh
Copy link
Contributor

@rwemay get what you are after, and agree this needs to happen for as much as possible, at least for Query/Modify/Compute methods. Think this is a no-brainer that just need to get done when someone has time to go through and fix it all. This can be either with the IElement interface or the various IAnalytical interfaces (IPanel, ILink, INode etc) depending on what is more suitable for the case.

Think Create will be trickier though. As highlighted by @pawelbaran, we can not initial interface types (be it IPanel or IElement). The two options of helping with this I can think of are:

  1. Add discipline enum as mentioned by @pawelbaran . Not a super big fan of this myself as it is not really scale-able. Would have to come back to both the enum as well as all the methods any time something is added.
  2. Add some helper methods for generating the geometry needed then add small create methods for each panel type. Will end up with more methods doing this, but still prefer it. Can try to keep any logic in Geometry/Common/Analytical engine and just a minor method calling it from Structure/Environment/WhatEversNext Engines.

And regarding you last point for list management, I would agree. Would like to have a generic AddToListProperty as well as ClearListProperty method in the reflection engine. Would remove the AddOpening type of method all in all (as not needed in the code at all).

@FraserGreenroyd
Copy link
Contributor

  • AddOpening - I initially thought we could remove this from Environment object and just use 'set property' but I suspect this has been added so you can append openings to a panel (instead of replacing the list)? It made me think we might want some form of 'append' alongside SetProperty for this type of case. Could be tricky with nested lists though.

This adds openings based on an algorithm of working out which openings sit on which panel, rather than setting the property of an individual panel.

@pawelbaran
Copy link
Member

@FraserGreenroyd do you use the BH.Engine.Common.Compute.DistributeOutlines?

Not sure if applicable in that particular case, but it is quite powerful and robust. I believe it would be good to wrap all panel constructors and opening assignment around it, both in the BHoM_Engine as well as in interop packages in order to provide similar behaviour for all - what do you think?

@FraserGreenroyd FraserGreenroyd removed their assignment Nov 18, 2019
@pawelbaran pawelbaran removed their assignment Jan 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:question Ask for further details or start conversation
Projects
None yet
Development

No branches or pull requests

4 participants