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

Dynamo 2.0 Dictionary watch window beneath ZT nodes doesn't display #8885

Closed
ThomasMahon opened this issue May 23, 2018 · 17 comments
Closed
Labels

Comments

@ThomasMahon
Copy link

ThomasMahon commented May 23, 2018

Dynamo version

image

Operating system

Win 10

What did you do?

Dictionary watch window doesn't display beneath a node.

recording 12

Seems to only be affecting either ZT nodes, or generic dictionaries:
image

@ThomasMahon ThomasMahon changed the title Dynamo 2.0 Dictionary watch window beneath ZT node doesn't display Dynamo 2.0 Dictionary watch window beneath ZT nodes doesn't display May 23, 2018
@mjkkirschner
Copy link
Member

@aparajit-pratap was this fixed?
#8734

@mjkkirschner
Copy link
Member

@ThomasMahon is it possible to test on the latest daily build?

@ThomasMahon
Copy link
Author

ThomasMahon commented May 23, 2018

@mjkkirschner unfortunately its still not working on the latest daily:
image

recording 13

@mjkkirschner
Copy link
Member

@ThomasMahon can you show an image of the data from both keys of the dictionary?
Are there nested dictionaries?

@mjkkirschner
Copy link
Member

also @ThomasMahon just curious if you see this with your own nodes only or other ZT nodes as well? I feel like Déjà vu.

@ThomasMahon
Copy link
Author

@mjkkirschner no nested dictionaries, 2D list of curves, and 1D list of strings. Maybe its caused by using generic types in the dictionary declaration due to the mixed return types, given that the OOTB node shown above is working?

image

@mjkkirschner
Copy link
Member

mjkkirschner commented May 23, 2018

@ThomasMahon tough to say without the source for the nodes - but can you try removing the nested generics so type your dictionary <string,object> and then also or in another experiment remove the [] from the key names.

@ThomasMahon
Copy link
Author

ThomasMahon commented May 23, 2018

@mjkkirschner quick test (simply returns whats input via a dict), and it looks like the culprit is the [] brackets in the outport names:

image

I'm a bit of a perfectionist, so I would be a bit disheartened if I have to remove this notation given how useful it is to 'initiated' users (plus, it should be a convention, which I believe is already in use with most OOTB nodes).

What causes the problem out of interest?

@aparajit-pratap
Copy link
Contributor

@ThomasMahon I don't see there's a problem with [] in the port names either as long as they are written in the MultiReturnAttribute as well.

@aparajit-pratap
Copy link
Contributor

Hmm, I tried it with a generic type too:
image
image

@ThomasMahon
Copy link
Author

ThomasMahon commented May 23, 2018

@aparajit-pratap So the only difference I can see is no type declaration when I instantiate the list of inputs for the MultiReturn attribute:

image

I went ahead and tested the node using new string[] instead and it still fails.

Here's the code:

        /// <summary>
        ///  Dictionary
        /// </summary>
        /// <param name="elements">elements</param>
        /// <returns name="Element">Element</returns>
        [MultiReturn(new[] {"Element[]", "Element1[][]"})]
        public static Dictionary<string, List<DynamoElement>> Dictionary2(List<DynamoElement> elements)
        {
            return new Dictionary<string, List<DynamoElement>>
            {
                {"Element[]", elements },
                {"Element1[][]", elements }
            };
        }

Also, why do the first set of square brackets not show on the outport?

image

@ThomasMahon
Copy link
Author

...maybe its my XML tags?

@ThomasMahon
Copy link
Author

Yeah, its the XML tags:
image

So in v2.0 the MultiReturn attribute list, the Dictionary keys AND the XML tags all have to match otherwise this problem occurs. Is this documented anywhere?

@aparajit-pratap
Copy link
Contributor

@ThomasMahon could you also share how you rewrote your xml tags in order to get it to work?

@mjkkirschner this looks like a bug to me. It seems that the xml tags conflict with the multi-return attribute in terms of port naming and since the node preview for multi-return nodes depends on port names, the preview is messed up as well. I think it should be ok to expect the multi-return attribute to match with the keys but it's a bit too much to expect the xml tags to match with everything else too. I think we should therefore make an exception for multi-return nodes and ignore the presence of xml tags in determining their port names. Thoughts?

@ThomasMahon
Copy link
Author

ThomasMahon commented May 24, 2018

@aparajit-pratap sure:

        /// <summary>
        ///  Dictionary
        /// </summary>
        /// <param name="elements">elements</param>
        /// <returns name="Element[]">Element[]</returns>
        /// <returns name="Element1[][]">Element[][]</returns>
        [MultiReturn(new[] { "Element[]", "Element1[][]" })]
        public static Dictionary<string, List<DynamoElement>> Dictionary2(List<DynamoElement> elements)
        {
            return new Dictionary<string, List<DynamoElement>>
            {
                {"Element[]", elements },
                {"Element1[][]", elements }
            };
        }

I'll reopen this for the time being. If this change isn't a bug, its quite a punitive one; previous versions of Dynamo didn't enforce it as you mentioned.

@ThomasMahon ThomasMahon reopened this May 24, 2018
@aparajit-pratap
Copy link
Contributor

@jnealb could you log this as a bug? The basic issue is that the xml tags conflict with the multi-return attributes for port naming. I think we should ignore the presence of xml tags in determining port names for multi-return nodes as those are already determined by the attributes.

@jnealb
Copy link
Collaborator

jnealb commented Jun 1, 2018

@aparajit-pratap This is filed as QNTM-4403, please update with details if necessary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants