Refactoring of Dynamo Toolkit to better leverage the new version of BHoM_UI

adecler commented 4 years ago

NOTE: Depends on

Issues addressed by this PR

Closes #265

This is a similar exercise than the one done for Excel and Grasshopper.

I will add any additional issues fixed by this PR as I go along

Closes #140 Closes #146 Closes #162 Closes #194 Closes #207 Closes #226 Closes #239 Closes #241 Closes #243 Closes #249 Closes #261 Closes #264

Test files

Changelog

Additional comments

Note, that some components like Create.Point are failing for me on Revit 2019 only. Some native component are failing too. After looking further into this, I realised that this was happening even without the BHoM installed so not sure what is causing it. See issue #266
The centralisation of the dlls is only done for 2.3 as it seems that feature is not well supported in 2.0

pawelbaran commented 4 years ago

@adecler could you please let us know when ready for review? Can't wait! 🍾

adecler commented 4 years ago

@pawelbaran, I suggest you switch to this branch already to provide a continuous review. The plan is to have this PR ready by the end of the first sprint but I will keep it in a state that is always usable as is.

pawelbaran commented 4 years ago

I have just tried a build on today's Alpha combined with this branch. Dynamo 2.3 (Revit 2020) crashes when I open a new Dynamo file. I get the following exception:

A breakpoint on GlobalSearchMenu.Activate does not get hit at all.

@adecler do you have any idea what can go wrong here?

adecler commented 4 years ago

It might be related to the fact that we migrated to 4.0 ? I can see that your error is still referencing 3.0 so something is wrong there. Can you make sure your local copy of the branch is up to date ?

This is what you should have on the Dynamo Roaming folder:

pawelbaran commented 4 years ago

Reinstall of the latest Alpha + rebuild fixed the issue. I will keep on testing, thanks!

adecler commented 4 years ago

Warning: If you update your code using an installer, make sure to delete teh content of those two fodlers manually:

C:\Users\adecler\AppData\Roaming\Dynamo\Dynamo Core\2.3\packages\BHoM\bin
C:\Users\adecler\AppData\Roaming\Dynamo\Dynamo Revit\2.3\packages\BHoM\bin

The reason for that is because this PR is now using the ProgramData folder to find its BHoM dlls. Since the installer still copies all dlls in the folders above, you might end up with dlls of different versions being loaded. Resulting in really weird bugs.

adecler commented 4 years ago

I had to make a change in BHoM_UI for the ParamSelector menu to stay in sync after clicking on the - buttons on the component.

You can find the PR here: https://github.com/BHoM/BHoM_UI/issues/320

There is no direct dependency so PRs can be merged independently but good to switch to that one too when you are doing the testing.

adecler commented 4 years ago

I believe this PR is now ready for review. I did some initial testing following the testing procedure for Dynamo and all seems to working ok. No doubt we will find plenty of bugs once stress tested more heavily though :smile:.

Personally, I have found two problems so far:

Explode warning messages don't go away when the outputs have changed. That's a weird one. it's linked to Dynamo really unhappy when you change its output parameters. I looked into it but not sure how to fix it tbh. Notice that copy/pasting the Explode component remove the warning so there is probably a way to get the warning message without having to do that.
No way to force list on object input (e.g. Custom Object). That would be the equivalent of selecting List in the input contextual menu in Grasshopper. Maybe there is already a ways to do that in Dynamo but I couldn't figure this out. The best solution would probably be to leverage the new dialog window for editing the input name by adding a toggle for list as well.

Also be aware that I have removed the menu to select outputs as there are too many bugs on the Dynamo side for it to be worth it. I managed to get the input selection working though.

I don't pretend the BHoM will be as good in Dynamo as it is in Grasshopper but it looks like it will now be able to exist without us being ashamed of it. When you review this PR, keep in mind that I would rather remove a non essential feature than to keep a buggy version of it.

pawelbaran commented 4 years ago

In general LGTM, well done @adecler! I've found 2 issues that would it be nice to cover before merging:

For some reason I am not able to Explode nor query the Location of a pulled wall - please see the PullWall.rvt and PullWall.dyn files here.
Actually the second point you've mentioned in your comment: forcing list input is possible by managing input levels, which can be switched on by clicking on the arrow as in the screenshot and ticking Use Levels option: From what I have tested, the input levels are managed differently for BHoM and Dynamo (please see ListManagement.dyn from here). Based on my experience, the approach used by Dynamo is quite hard to capture (particularly for the ones grown up on GH trees), but in this case I think it would be fair to say that BHoM should simply mimic the way in which Dynamo handles the inputs (or even better, make Dynamo do that). As an extra reference, you could have a look at the relevant chapter of Dynamo Primer, particularly the List@Level paragraph.

It would be great to cover the above in this PR, but I would also be ready to approve as is and resolve the missing bits in the follow-up threads.

kayleighhoude commented 4 years ago

@adecler looking good so far. One thing I noticed, when using the global menu it works for adapters, engine methods, types but does not work specifically for datasets. I've tried this with multiple datasets. Not sure if this occurred for you too @pawelbaran DatasetsGlobalMenu

adecler commented 4 years ago

Thanks @pawelbaran ,

I have fixed the issue you found with Explode. Actually it was linked to output setters not covering for Exceptional cases like conversion th DesignScript failing or handling of nulls. That's probably a point worth mentioning: there is no easy way to keep the object as BHoM when outputting them and only converting when requested as input (like in Grasshopper). This means that we always convert to Design script so a lot more susceptible to conversions errors.

As for your second point, it looks like it will require a bit more time than I can afford this sprint. I have raised a spearate issue for that here: https://github.com/BHoM/Dynamo_Toolkit/issues/271

adecler commented 4 years ago

Thanks @kayleighhoude , this should now be fixed.

pawelbaran commented 4 years ago

I ran another round of tests trying to break the thing. It was not easy, well done 👍

Concerning the 1st point of my previous review, Explode and GetProperty works now, but they still turn yellow + the output geometry does not cast to DesignScript (i.e. it is useless for a regular user who does not know anything about BHoM vs DesignScript casting). Not sure if anything could be done about it, taken into account your previous comment, but would be great to fix that or at least leave an informative error if possible.

adecler commented 4 years ago

@pawelbaran, the error message is as follows:

Engine.Reflection.Compute.RecordWarning("Failed to convert output " + index + " to DesignScript. Returning it without conversion.\nError: " + e.Message);

I am happy to change it to whatever you feel is more appropriate.

Keep in mind that error/behaviour is only happening if the conversion methods are failing to create a valid DesignScript object. So the best way to improve this is to improve the conversion methods themselves. That is very much outside my confort zone as I have never used those objects myself. So this approach is to make sure I at least return something more useful than a null (or even worst in the previous case, cause all the outputs to fail).

pawelbaran commented 4 years ago

Good point @adecler, I assumed the error comes from the UI, not the Engine, apologies. The error comes from the line below, which should also contain a null check for InternalBoundaries:

https://github.com/BHoM/Dynamo_Toolkit/blob/d202ceedee5e4ee4198f7acfb09c364e38888191/Dynamo20/Dynamo_Engine/Convert/ToDesignScript.cs#L232

The above should be:

if (surface.InternalBoundaries != null && surface.InternalBoundaries.Count != 0)

I think it could be included by you in this PR, but happy to open a new one after this gets merged - up to you.

adecler commented 4 years ago

@pawelbaran , I have added basic null checks on all ToDesignScript and FromDesignScript methods to make you life a bit easier when you review those methods. Feel free to edit as you see fit.

adecler commented 4 years ago

LGTM now, but I would be way more comfortable if we had more than one approving review before merging this PR - the scope of changes is massive, cross-check would be more than welcome.

Fully agreed. I suppose the most important review to get would be from @kayleighhoude . Kayleigh, would it be possible for you to do another review ?

adecler commented 4 years ago

/azp run Dynamo_Toolkit.CheckInstaller

azure-pipelines[bot] commented 4 years ago

Azure Pipelines successfully started running 1 pipeline(s).

adecler commented 4 years ago

Thanks @kayleighhoude,

This is now fixed. Thanks to Dynamo requiring us to provide individual methods for each parameter count in order for lacing to work, we had only covered up to 12 parameters. Doing a quick analysis of the BHoM methods, I noticed that we have one method with 35 parameters and one with 27. So I decided to cover up to 40 parameters just to be safe. Thanks god for Grasshopper to generate that code automatically through scripting !

pawelbaran commented 4 years ago

/azp run Dynamo_Toolkit.CheckInstaller

azure-pipelines[bot] commented 4 years ago

Azure Pipelines successfully started running 1 pipeline(s).

BHoM / Dynamo_UI