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

BHoM_UI for the Grasshopper_Toolkit - Issue 226 - Input arrays #117

Closed

Conversation

epignatelli
Copy link
Member

@epignatelli epignatelli commented Aug 1, 2019

Issues addressed by this PR

Closes BHoM/Grasshopper_UI#226

Test files

Note that, even you need to use BHoM/BHoM_Engine#1124 to be able to input arrays, although it does not represent a code-wise dependency.
https://burohappold.sharepoint.com/:f:/s/BHoM/EjmxIfvj4WNBvi9qWzVvunoBwjYJX03lohJTTu8WHAxAhA?e=JpiJCu

Changelog

  • Adds public abstract T[] DataAccessor.GetDataArray<T>(int index);

Additional comments

This pr is to support BHoM/Grasshopper_UI#389. It adds an array getter. The method is abstract so, it must be implemented.

Verified

This commit was signed with the committer’s verified signature.
dmitryax Dmitrii Anoshin
@epignatelli epignatelli requested a review from adecler August 1, 2019 14:24
@epignatelli epignatelli self-assigned this Aug 1, 2019
@epignatelli epignatelli added severity:critical No workaround exists. Essential to continue type:feature New capability or enhancement labels Aug 1, 2019
@epignatelli epignatelli added this to the BHoM 2.4 β MVP milestone Aug 1, 2019
@epignatelli epignatelli marked this pull request as ready for review August 1, 2019 14:26
epignatelli pushed a commit to BHoM/Dynamo_UI that referenced this pull request Aug 1, 2019
@epignatelli epignatelli changed the title Solves https://github.com/BHoM/Grasshopper_Toolkit/issues/226 BHoM_UI for the Grasshopper_Toolkit - Issue 226 - Input arrays Aug 1, 2019
@adecler
Copy link
Member

adecler commented Aug 3, 2019

As discussed briefly, I cannot approve this PR as I disagree with the principle behind it.

All our UIs have a clear mapping for 0d, 1d and 2d data structures and this PR is breaking that.
If your external library is using arrays, you need to wrap it so it exposes only lists. This is important in order to not let external complexity creep in into our framework. To me, this case is the same as if you were suddenly trying to introduce something like pointers because your external library requires it.

Let's have a call next week if you need help wrapping your external methods.

@adecler adecler added the status:do-not-merge For instance, test PR, for discussion, or dependant PRs not ready for merge label Aug 3, 2019
@epignatelli
Copy link
Member Author

epignatelli commented Aug 8, 2019

closing this, for the moment, after our discussion @adecler
The functionality required will be replaced by a conversion to list.

I will check if performance becomes an issue, but I don't think it will.

@epignatelli epignatelli closed this Aug 8, 2019
@epignatelli epignatelli deleted the Grasshopper_Toolkit-Issue226-InputsArrays branch October 11, 2019 13:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
severity:critical No workaround exists. Essential to continue status:do-not-merge For instance, test PR, for discussion, or dependant PRs not ready for merge type:feature New capability or enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Grasshopper_Toolkit: Cannot input an array of objects
2 participants