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

Handle HashSet, FragmentSet,... as input list #253

Merged
merged 3 commits into from
May 8, 2020

Conversation

adecler
Copy link
Member

@adecler adecler commented May 6, 2020

NOTE: Depends on

BHoM/BHoM#853
BHoM/BHoM_Adapter#228
BHoM/BHoM_Engine#1743

(at least for testing)

Issues addressed by this PR

Closes #154

IEnumerable types are can now be properly inputted as lists:

image

Test files

Use any CreateObject component as add inputs for Tags and Fragments.

Changelog

Additional comments

I had to do a second commit to fix a problem created from a previous PR: Added inputs to a CreateObject component would not work properly after copy paste or reopening the file.

@adecler adecler added the type:bug Error or unexpected behaviour label May 6, 2020
@adecler adecler added this to the BHoM 3.2 β MVP milestone May 6, 2020
@adecler adecler self-assigned this May 6, 2020
pawelbaran
pawelbaran previously approved these changes May 6, 2020
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.

LGTM, tested on autoconstructors of objects with HashSet properties, works fine.

Copy link
Contributor

@IsakNaslundBh IsakNaslundBh left a comment

Choose a reason for hiding this comment

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

Tested, and works fine as long as everything is plugged in, but found some cases that crashes out:

  1. Adding a fragments input and leaving it unplugged makes the component go red and give a "1. Solution exception:Object reference not set to an instance of an object." error. I know you can just remove the input and that it only happens if a user explicitly has added it, but think it would be good for this to just work.

image

  1. Adding two of the same fragment type crashes with the same "1. Solution exception:Object reference not set to an instance of an object." error. This arguably should be crashing, but an error like "Can only add one fragment of a particular type to a BHoMObject" or similar instead of a null reference exception would be better I think:

image

Apart from that, things seem to be working well. Also love that fragments now work in the explode! 👍

@adecler
Copy link
Member Author

adecler commented May 7, 2020

Thanks @IsakNaslundBh ,

  • The issue of the default input turning the component red is actually a Grasshopper problem created by the changes here. I have fixed it in Take care of input type that are both list and items Grasshopper_UI#505
  • The second problem you raised was caused by something that have been annoying me for a while now so I am glad to have an opportunity to fix it 😄. (see new commit)
    image

Can you do a final review and approve all five PRs if you're happy with it ?

Copy link
Contributor

@IsakNaslundBh IsakNaslundBh left a comment

Choose a reason for hiding this comment

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

Issues I had got solved with the latest commit(s), and have not found any problems when using the UI!

@adecler
Copy link
Member Author

adecler commented May 8, 2020

/azp run BHoM_UI.CheckInstaller

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@adecler adecler merged commit b2f76d1 into master May 8, 2020
@adecler adecler deleted the BHoM_UI-#154-FixXSetInputsAsList branch May 8, 2020 02:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:bug Error or unexpected behaviour
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tags cannot be provided as input in the CreateObject
3 participants