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

Remove the ability to create typed objects from the CreateCustom component #160

Merged
merged 1 commit into from
Nov 27, 2019

Conversation

adecler
Copy link
Member

@adecler adecler commented Nov 12, 2019

Issues addressed by this PR

Closes #159

Test files

Just make sure that the CreateCustom component is not allowing you to select a type from its menu and that it is still working for creating custom objects

Additional comments

Make sure to also check the PR in Excel_Toolkit as some changes were needed there to keep it compiling

Copy link
Member

@epignatelli epignatelli left a comment

Choose a reason for hiding this comment

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

Thanks @adecler !

The BHoM_UI works ok by itself, but modifying the logic here breaks the CreateCustom behaviour in Grasshopper at least.

Selecting a different type from the parameter menu for a specific input does not work anymore. To test it:

  1. Create a CreateCustom component
  2. Add two inputs
  3. Create a panel with the value true
  4. Input the panel in both the inputs of the component
  5. Change the TypeHint of the first input to int
  6. If you use an Explode, you'll see that the change has no effects

I think the fix is just to check for type changes as well in grasshopper to decide when to recompute the solution.
Specifically, I think that these lines should add a check for the type:

            bool recompute = this.Phase == GH_SolutionPhase.Computed
                             && !e.Parameter.Sources.Any(p => p.Attributes.GetTopLevel.DocObject is ExplodeComponent)
                             && e.Parameter.NickName != Caller.InputParams[e.ParameterIndex].Name
                             && Engine.Grasshopper.Query.Type(e.Parameter) != Caller.InputParams[e.ParameterIndex].DataType;

@adecler
Copy link
Member Author

adecler commented Nov 15, 2019

@epignatelli , weird, it works perfectly fine for me:

image

Which makes sense because nothing in the code I have changed on this PR could trigger that effect. Are you sure you are on the correct version for all repos?

@epignatelli
Copy link
Member

Yeah, that's very weird, but I agree with you, I was surprised in the first place.

What I don't actually understand is how it can be working now, without that check.
I'll re-check and make sure the branch set is correct!

@epignatelli epignatelli self-requested a review November 17, 2019 18:23
Copy link
Member

@epignatelli epignatelli left a comment

Choose a reason for hiding this comment

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

I am not sure why it was not working before.
I checkout the latest masters, recompiled and now everything seems fine!

@epignatelli
Copy link
Member

/azp BHoM_UI.CheckCore

@azure-pipelines
Copy link

Command 'BHoM_UI.CheckCore' is not supported by Azure Pipelines.

Supported commands
  • help:
    • Get descriptions, examples and documentation about supported commands
    • Example: help "command_name"
  • list:
    • List all pipelines for this repository using a comment.
    • Example: "list"
  • run:
    • Run all pipelines or a specific pipeline for this repository using a comment. Use this command by itself to trigger all related pipelines, or specify a pipeline to run.
    • Example: "run" or "run pipeline_name"
  • where:
    • Report back the Azure DevOps orgs that are related to this repository and org
    • Example: "where"

See additional documentation.

@epignatelli
Copy link
Member

/azp run BHoM_UI.CheckCore

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@epignatelli
Copy link
Member

/azp run BHoM_UI.CheckInstaller

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@epignatelli
Copy link
Member

@adecler is there anything that prevents us from merging this?

@adecler
Copy link
Member Author

adecler commented Nov 20, 2019

@epignatelli , yes, Excel_Toolkit needs to be updated first as this PR was preventing it from compiling.

I did a quick attempt to fix it in https://github.com/BHoM/Excel_Toolkit/pull/170 but it was apparently a bit too quick 😋 . @awakeman is now on it to do that properly.

@epignatelli
Copy link
Member

epignatelli commented Nov 20, 2019

Ah, right, ok! I'll add a do-not-merge then, waiting for BHoM/Excel_Toolkit#170 to be closed.

Just wanted to prevent an accidental "Click the green button" automatism :D

@epignatelli epignatelli added the status:do-not-merge For instance, test PR, for discussion, or dependant PRs not ready for merge label Nov 20, 2019
@adecler adecler removed the status:do-not-merge For instance, test PR, for discussion, or dependant PRs not ready for merge label Nov 27, 2019
@adecler adecler merged commit 611d560 into master Nov 27, 2019
@adecler adecler deleted the BHoM_UI-#159-RemoveSelectedItemFromCreateCustom branch November 27, 2019 08:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:compliance Non-conforming to code guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove the ability to create typed objects from the CreateCustom component
2 participants