[DYN-7838] Color range Node: Show Input ports with default value#15715
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
See the ticket for this pull request: https://jira.autodesk.com/browse/DYN-7838
UI Smoke TestsTest: success. 11 passed, 0 failed. |
QilongTang
left a comment
QilongTang
left a comment
There was a problem hiding this comment.
LGTM, do you mind adding a unit test? Just to test the default behavior of the node, it looks like such baseline might be missing
| [JsonConstructor] | ||
| private ColorRange(IEnumerable<PortModel> inPorts, IEnumerable<PortModel> outPorts) : base(inPorts, outPorts) | ||
| { | ||
| if (inPorts.Count() == 3 && outPorts.Count() == 1) |
There was a problem hiding this comment.
@QilongTang I think we should come up with something better than this pattern - for example default value attributes or something where the author only needs to specify the default value once and not need to consider it in their json constructor unless they want to.
|
|
||
| // If there are colors supplied | ||
| if (InPorts[0].IsConnected) | ||
| if (InPorts[0].Connectors.Any()) |
| public class ColorRange : NodeModel | ||
| { | ||
| private List<Color> _defaultColors; | ||
| private List<Color> DefaultColors => _defaultColors ??= DefaultColorRanges.Analysis.ToList(); |
There was a problem hiding this comment.
it does not look like you use any dynamic resizing list functionality here, is there a reason you use this type here and in the create helpers?
There was a problem hiding this comment.
CreateParametersForColors() was already accepting List and didn't want to chage existing code unless I really have to. Hepe the latest commit is okay.
|
@ivaylo-matov Can you address the questions in this PR, so that we can move forward with the review ? Thanks. |
@zeusongit , I can see one question and I have responded to it |
Oh I was referring to this: #15715 (comment) |
@zeusongit If the question is why I've replaced IsConnected with Connectors.Any(), the reason is that after adding the AssociativeNodes, .DefaultValue, and .UsingDefaultValue, the .IsConnected property will always return true, even when nothing is connected. So I used Connectors.Any() to check for connections. |
| AstFactory.BuildFunctionCall( | ||
| new Func<int, int, int, int, Color>(DSCore.Color.ByARGB), | ||
| new List<AssociativeNode> | ||
| { |
There was a problem hiding this comment.
Missed the Opacity, A here resulting in that issue. Fixing in 3.6.1 with a PR.
5 participants
Purpose
This PR aims to address DYN-7838 where two input ports (colors and indices) were not showing as blue to indicate they have default values. Default values are now assigned during initialization, and the ports can toggle between using default values and user-supplied inputs.
I've added initialization for default values
DefaultColorsNodeandDefaultIndicesNode.Updated the initialization logic for input ports to correctly display their default state.
Users can still override default values when needed.
Declarations
Check these if you believe they are true
*.resxfilesRelease Notes
This PR aims to address DYN-7838 where two input ports (colors and indices) were not showing as blue to indicate they have default values. Default values are now assigned during initialization, and the ports can toggle between using default values and user-supplied inputs.
Reviewers
@QilongTang
@reddyashish
FYIs
@dnenov