Skip to content

Update NodeModel Autocomplete Match Criteria #11621

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.

Sign up for GitHub

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

Jump to bottom
Closed
QilongTang wants to merge 1 commit into from
Closed

Update NodeModel Autocomplete Match Criteria #11621

QilongTang wants to merge 1 commit into from

Conversation

@QilongTang
Copy link
Contributor

@QilongTang QilongTang commented Apr 16, 2021 •
edited
Loading

Please Note:

  1. Before submitting the PR, please review How to Contribute to Dynamo
  2. Dynamo Team will meet 1x a month to review PRs found on Github (Issues will be handled separately)
  3. PRs will be reviewed from oldest to newest
  4. If a reviewed PR requires changes by the owner, the owner of the PR has 30 days to respond. If the PR has seen no activity by the next session, it will be either closed by the team or depending on its utility will be taken over by someone on the team
  5. PRs should use either Dynamo's default PR template or one of these other template options in order to be considered for review.
  6. PRs that do not have one of the Dynamo PR templates completely filled out with all declarations satisfied will not be reviewed by the Dynamo team.
  7. PRs made to the DynamoRevit repo will need to be cherry-picked into all the DynamoRevit Release branches that Dynamo supports. Contributors will be responsible for cherry-picking their reviewed commits to the other branches after a LGTM label is added to the PR.

Purpose

Per https://jira.autodesk.com/browse/DYN-3470.

For NodeModel nodes, output types are defined by user which usually do not contain namespace

example

    [IsDesignScriptCompatible]
    [NodeName("Color Range")]
    [NodeCategory("Core.Color.Create")]
    [NodeDescription("ColorRangeDescription",typeof(Resources))]
    [NodeSearchTags("ColorRangeSearchTags", typeof(Resources))]

    [InPortNames("colors", "indices", "value")]
    [InPortTypes("DSCore.Color[]", "double[]", "double")]
    [InPortDescriptions(typeof(Resources),
        "ColorRangePortDataColorsToolTip",
        "ColorRangePortDataIndicesToolTip",
        "ColorRangePortDataValueToolTip")]
    [OutPortNames("color")]
    [OutPortTypes("Color")]
    [OutPortDescriptions(typeof(Resources),
        "ColorRangePortDataResultToolTip")]
    [AlsoKnownAs("DSCoreNodesUI.ColorRange")]

Since the OutPortType(OutputParameter) is Color instead of DSCore.Color, it was not a exact match for node autocompete. While we can define output type with namespace there, in order to be consistent on simplified PortType name display in library, we make it tolerant here.

Result:
NodeModel nodes like Color.Range and Color.Pallette now appear in NodeAutoComplete Suggestions
image

Declarations

Check these if you believe they are true

  • The codebase is in a better state after this PR
  • Is documented according to the standards
  • The level of testing this PR includes is appropriate
  • User facing strings, if any, are extracted into *.resx files
  • All tests pass using the self-service CI.
  • Snapshot of UI changes, if any.
  • Changes to the API follow Semantic Versioning and are documented in the API Changes document.
  • This PR modifies some build requirements and the readme is updated

Reviewers

@DynamoDS/dynamo

FYIs

aparajit-pratap
aparajit-pratap reviewed Apr 16, 2021
View reviewed changes
src/DynamoCoreWpf/ViewModels/Search/NodeAutoCompleteSearchViewModel.cs
// e.g. For ColorRange, the OutPortType(OutputParameter) is Color instead of DSCore.Color
// While we can define output type with namespace there, in order to be consistent on simplified
// PortType name display in library, we make it tolerant here
if (element.OutputParameters.Any(op => inputPortType.EndsWith(op)))
Copy link
Contributor

@aparajit-pratap aparajit-pratap Apr 16, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

This could result in incorrect matches. Shouldn't it be an exact match? I think the only way to achieve that is to impose full namespaces for both input and output port types. For example, this will give a false match if the output port type is Color from DSCore namespace for one node and another node expects PackageX.Color or just Color but from a different namespace.

Copy link
Contributor Author

@QilongTang QilongTang Apr 16, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@aparajit-pratap Sure, please see the reasoning I mentioned.

  1. Our library does not display full type either, e.g. for Color.Red, the out put type displayed in library is just Color instead of DSCore.Color. Right now, Color.Range is following the same. If we make it exact match everywhere, should we use the full type everywhere? Otherwise, NodeModel nodes will display different level of info in library than ZT nodes. The false match case right now arguably is already misleading in library.
    image

image

  1. Even if it is mismatch, it could still be valuable to be listed as potential.

If we all prefer strong match, I would say we have some systematic changes to make. I am open for what you guys prefer.

Copy link
Contributor

@aparajit-pratap aparajit-pratap Apr 16, 2021

Choose a reason for hiding this comment

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

Well, I think it's better to be verbose than incorrect. For a correct match, we must have exact matches with full namespaces.

Copy link
Contributor Author

@QilongTang QilongTang Apr 16, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@aparajit-pratap I interpret it as you are in supportive of full type names everywhere in Dynamo then? Otherwise on book, Color.Range can't be suggested to Color.Red which is the current behavior

Copy link
Contributor

@aparajit-pratap aparajit-pratap Apr 16, 2021

Choose a reason for hiding this comment

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

I interpret it as you are in supportive of full type names everywhere in Dynamo then

Yes

Copy link
Contributor Author

@QilongTang QilongTang Apr 16, 2021

Choose a reason for hiding this comment

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

@aparajit-pratap @mjkkirschner Well, we do not have a choice I believe, if we touch the OutputType of NodeModelNodes, they will be reflected in Library. As a result, Color.Range will display output as DSCore.Color which is different than Color.Red. That was the reason why I started to think of alternatives like this PR.

Copy link
Contributor

@Amoursol Amoursol Apr 16, 2021

Choose a reason for hiding this comment

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

If there are going to be major functionality changes (i.e. how it displays in the Library) can we set up a quick meeting to discuss? I would also like to have Jingyi present too :)

Copy link
Member

@mjkkirschner mjkkirschner Apr 16, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@QilongTang is that correct? - for example -
Screen Shot 2021-04-16 at 5 33 23 PM

This is a ZT node of course, but it does not show the fully qualified type name, do NodeModel nodes act differently? If so that sounds like something to make consistent.

Copy link
Contributor Author

@QilongTang QilongTang Apr 18, 2021

Choose a reason for hiding this comment

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

@mjkkirschner See the screenshot I provided above for ColorRange, input is DSCore.Color[] and output is Color.

Copy link
Member

@mjkkirschner mjkkirschner Apr 20, 2021

Choose a reason for hiding this comment

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

yes, thats my point @QilongTang - your example is a NodeModel - maybe it should behave like the zero touch node's in the library (my screenshot) - instead. Or vice versa.... though lots of repeated information seems superfluous.

@QilongTang QilongTang mentioned this pull request Apr 19, 2021
Merged
8 tasks
@QilongTang
Copy link
Contributor Author

QilongTang commented Apr 23, 2021

Closing this PR since a preferred alternative solution has been discussed

@QilongTang QilongTang closed this Apr 23, 2021
@QilongTang QilongTang deleted the NodeModelNodesAutoCompleteEdgeCase branch April 28, 2021 15:32
@QilongTang QilongTang restored the NodeModelNodesAutoCompleteEdgeCase branch April 28, 2021 15:32
@QilongTang QilongTang deleted the NodeModelNodesAutoCompleteEdgeCase branch September 8, 2021 14:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mjkkirschner mjkkirschner mjkkirschner left review comments

@aparajit-pratap aparajit-pratap aparajit-pratap left review comments

@Amoursol Amoursol Amoursol left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@QilongTang @mjkkirschner @aparajit-pratap @Amoursol