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

Prevent a Rhino crash when an old type is deserialised in the CreateCustomObject component #405

Merged
merged 2 commits into from
Sep 24, 2019

Conversation

epignatelli
Copy link
Member

Issues addressed by this PR

Closes #396

Rhino crashes altogether if you open a script with a CustomObjecy component that serialised a Type that doesn't exist anymore in the BHoM.

Test files

@IsakNaslundBh would you mind using the file that triggered the issue in the first place?

Changelog

  • Make sure that Engine.Grasshopper.Query.Type(this IGH_Param param, Caller caller = null) never returns null.

Additional comments

This is quite critical, since it is crashing the whole Rhino.

@epignatelli epignatelli added severity:critical No workaround exists. Essential to continue type:bug Error or unexpected behaviour labels Sep 6, 2019
@epignatelli epignatelli added this to the BHoM 2.4 β MVP milestone Sep 6, 2019
@epignatelli epignatelli self-assigned this Sep 6, 2019
@epignatelli epignatelli changed the title Solves #396 Prevent a Rhino crash when an old type is deserialised in CustomObject Sep 6, 2019
@epignatelli epignatelli changed the title Prevent a Rhino crash when an old type is deserialised in CustomObject Prevent a Rhino crash when an old type is deserialised in the CreateCustomObject component Sep 6, 2019
@epignatelli
Copy link
Member Author

From the Sprint Closure call @IsakNaslundBh to have a go with the script creating the issue

@epignatelli
Copy link
Member Author

epignatelli commented Sep 18, 2019

@IsakNaslundBh shall we have a look at this together, tomorrow morning, in order to close it?

Copy link
Member

@adecler adecler 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 overall happy with that PR except for the one change I requested.

It is important to note that this happens because OnGrasshopperUpdates is called when the component is deleted, which is questionable. This is by our own design:

public class CreateCustomComponent : CallerComponent, IGH_VariableParameterComponent
    {
        /*******************************************/
        /**** Properties                        ****/
        /*******************************************/

        public override Caller Caller { get; } = new CreateCustomCaller();


        /*******************************************/
        /**** Constructors                      ****/
        /*******************************************/

        public CreateCustomComponent() : base()
        {
            this.Params.ParameterChanged += OnGrasshopperUpdates;
        }

So we might want to find an event more suitable than ParameterChanged or a way to differentiate the case where the component is deleted.

I would recommend sticking to this fix for now but raising another issue to investigate further.

if (param == null)
return typeof(object);
type = typeof(object);
Copy link
Member

Choose a reason for hiding this comment

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

This one needs to stay a return as it doesn't belong to the same if-else chain as the rest.

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.

Updated the code to @adecler s comments.

This solves the problem for me and stops the crash.

Copy link
Member

@adecler adecler left a comment

Choose a reason for hiding this comment

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

LGTM now

@adecler adecler merged commit 42acc24 into master Sep 24, 2019
@adecler adecler deleted the Grasshopper_Toolkit-#396-TypeCrashes branch September 24, 2019 12:36
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 type:bug Error or unexpected behaviour
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Grasshopper_Toolkit: Rhino crash with custom component with old types
3 participants