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

Migrating grids and levels from Architecture to Geometry.SettingOut #1204

Merged
merged 7 commits into from
Sep 18, 2019

Conversation

al-fisher
Copy link
Member

NOTE: Depends on

BHoM/BHoM#554

Issues addressed by this PR

Closes #1203

Test files

https://burohappold.sharepoint.com/sites/BHoM/02_Current/Forms/AllItems.aspx?RootFolder=%2Fsites%2FBHoM%2F02%5FCurrent%2F12%5FScripts%2F01%5FTest%20Scripts%2FBHoM%5FEngine%2FArchitecture%5FoM%2F%23536%20%2D%20migration%20of%20grid%20and%20level&FolderCTID=0x0120008122C8891F89054B8ACED0196C70DFC4

Changelog

  • BH.oM.Architecture.Elements.Grid and Level Creates deprecated.
  • Grid and Level added to BH.oM.Geometry.SettingOut
  • Environment methods based on Level deprecated and updated to utilise BH.oM.Geometry.SettingOut.Level

@al-fisher al-fisher added severity:low Doesn't stop/slow current workflow status:do-not-merge For instance, test PR, for discussion, or dependant PRs not ready for merge type:compliance Non-conforming to code guidelines labels Sep 14, 2019
@al-fisher al-fisher self-assigned this Sep 14, 2019
Copy link
Member

@michaldengusiak michaldengusiak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just rebuild all Toolkits as plan to test XML so in order to help will run this review as well...
I noticed provided scripts were referring to old data so I adjusted and saved working examples is same folder,

  1. TestGridCreateMethods - pass
    image

Copy link
Contributor

@FraserGreenroyd FraserGreenroyd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - normally I'd go with a @pawelbaran approach for calling new methods from the deprecated methods to save on maintaining both during the deprecation process but am happy for this to move through as it's the last component of 2.4MVP as we move into 2.4RC next week 😄

Copy link
Member

@pawelbaran pawelbaran left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry @al-fisher to block this one, but wasn't SettingOut meant to land in Common? That is what I understood from the brief.

As far as I can imagine grids and levels being related to architecture, I do not get why would they be a part of geometry - this namespace was historically limited to classes representing pure geometry: curves, surfaces etc., while Grids are objects with ICurve being only one of their properties, and Levels do not have geometry at all (of course they could be represented as XY planes with origin point with certain Z, but this is not their role in the object model).

I generally support the idea of creating SettingOut namespace in one of less discipline-specific repos than Architecture, but cannot think of a reason why this would it be Geometry and not e.g. Common.

@al-fisher
Copy link
Member Author

Sorry @al-fisher to block this one, but wasn't SettingOut meant to land in Common? That is what I understood from the brief.

Hey @pawelbaran - yep - do agree to some degree with some of the points you raise above - but do have a look at the original issue and here: BHoM/BHoM#536 (comment)
The Common namespace does need some work and crystallising in terms of its scope.
And as commented in the referenced issue above - I do wonder if we can be moving in the direction of objects finding appropriate homes which classify them less generically than just "Common" - a bit similar to not just relying on "Misc" as a placeholder.

That all said - I do agree with the potential for greater intuitiveness over where the Grid and Level sit.
As you say - they have to move out of Architecture to remove the transdiscipline interdependence.
One option that has been considered is a Building specific namespace as the objects we are talking about here (Grid and Level) are certainly "Building" objects.

There are however issues with simply introducing a transdiscipline Building namespace as to whether it is "transdiscipline" in that all disciplines are dependant on it - or vice versa (Building is dependant

Therefore I do propose that we do continue with BH.oM.Geometry.SettingOut as per the original issue - with the proviso that we could introduce a Building (or similar slot at a later date). The focus on backwards compatibility and auto-deprecation now will make this trivial. (@adecler FYI )

@pawelbaran does this make sense?

@pawelbaran
Copy link
Member

@al-fisher I get your point, but this still does not convince me.

From what you are writing, I understand that Geometry will probably not be the final namespace for the subject matter, we will need another iteration. That I guess is something that should be avoided not to break the existing scripts too often.

And even if the above is not an issue, I would still consider moving Grid and Level to Common - these classes just do not fit to what I understand by geometry. I believe it is still better to have a placeholder namespace than trying to push things into slots that they may not be suitable for.

What do you guys think @IsakNaslundBh @FraserGreenroyd?

@al-fisher
Copy link
Member Author

OK - two points on the above:

  • As above, the focus on backwards compatibility and auto-deprecation is now incredibly urgent and is being prioritised - so breaking scripts is increasing going to not be an issue and should not drive decisions long term (short term agree can do)

  • Right now, I do still think, Geometry, is the best location for geometrical Setting Out entities and concepts such as Grid and Level.
    Agree historically our Geometry namespace has been more focused on just primatives - like Curves and Solids etc. But think we could expand that. Which is why we have created the SettingOut sub-namespace

Does that help?

@FraserGreenroyd
Copy link
Contributor

What do you guys think @IsakNaslundBh @FraserGreenroyd?

Hmm it's a difficult one. On the one hand, I do agree with you @pawelbaran about the Geometry Namespace and whether it should contain objects which contain additional data beyond just geometric.

However, I also agree with @al-fisher that the Common namespace has yet to be defined and would not want to run the risk of it becoming the 'dumping ground' of objects which have no immediate better home.

I would agree with @al-fisher about the possibility of a Building namespace in which to store Building level objects, however, that is something which ought to have a reasonable discussion at the framework level (looping in @adecler for example) before being implemented. This isn't something which we would be able to achieve within the milestone (nor would we necessarily want to rush).

Equally, the joy of everything we're doing on deprecation allows us to make almost as many changes as we want without worrying about breaking scripts. Granted, it gives us more to manage in terms of deprecated code and working out when to remove it - but that's just something that comes with being maintainers of OS code.

So, to summarise... I agree with both of you (not helpful, I know), but lean towards agreeing with @al-fisher in terms of:

  • Merge in this block of work to ensure it unblocks the current rapid prototyping being done around the BHoM
  • Raise specific issues to discuss the framework level questions with @al-fisher , @adecler , @IsakNaslundBh , @FraserGreenroyd , @rwemay , @epignatelli etc. in terms of a Building (or other) namespace/best namespace for these objects - this can be done as part of the 2.5 milestone to ensure it isn't left while it's fresh in all our minds I'm sure
  • Refactor to move as appropriate following that discussion

Hope that helps, at least that's my thoughts in a rambling state - make of it what you will 😉

@pawelbaran
Copy link
Member

I get the point of you both, but still I would not agree with moving things around for the sake of doing it. At the moment I do not see any big difference between the Grid and Level sitting in Architecture, Common or Geometry, apart from the fact that in my understanding they are not geometrical primitives, so do not fit into the last. I also do not see what is being blocked by these two classes staying in Architecture for another iteration - why not park this action and move to Building, once it is agreed where it should sit?

Maybe the problem is me being excluded from any framework-level discussions (and still trying to maintain one of the core repos). I will not block this one any more, you guys do what you like.

Copy link
Member

@pawelbaran pawelbaran left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🏗

@al-fisher
Copy link
Member Author

Thanks @FraserGreenroyd - I wont comment on whether that is rambling or not 😄 - but I do think you did capture very well the overlapping considerations!
Thanks. 👍

And perfect - Thanks @pawelbaran
Yes - the really key point for me was indeed moving out of Architecture - so transdisciplinary workflows were not limited by hard coded chains of dependencies on Architecture.
Good to pick up on long term road map of common and other foundation namespaces as part of next milestone kick off.

Thanks all

@al-fisher al-fisher merged commit 3e9ccb8 into master Sep 18, 2019
@al-fisher al-fisher deleted the BHoM-#536-MigrateGridsFromArchitectureToGeometry branch September 18, 2019 12:07
@al-fisher al-fisher removed the status:do-not-merge For instance, test PR, for discussion, or dependant PRs not ready for merge label Sep 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
severity:low Doesn't stop/slow current workflow type:compliance Non-conforming to code guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update Create method for Grids and Levels from Architecture to Geometry.SettingOut
5 participants