-
Notifications
You must be signed in to change notification settings - Fork 636
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
[DYN-7838] Color range Node: Show Input ports with default value #15715
base: master
Are you sure you want to change the base?
[DYN-7838] Color range Node: Show Input ports with default value #15715
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See the ticket for this pull request: https://jira.autodesk.com/browse/DYN-7838
UI Smoke TestsTest: success. 11 passed, 0 failed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
@@ -42,6 +51,18 @@ protected virtual void OnRequestChangeColorRange() | |||
[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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
@@ -119,7 +150,7 @@ public ColorRange1D ComputeColorRange(EngineController engine) | |||
List<double> parameters; | |||
|
|||
// If there are colors supplied | |||
if (InPorts[0].IsConnected) | |||
if (InPorts[0].Connectors.Any()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
?
@@ -32,6 +32,15 @@ namespace CoreNodeModels | |||
[AlsoKnownAs("DSCoreNodesUI.ColorRange")] | |||
public class ColorRange : NodeModel | |||
{ | |||
private List<Color> _defaultColors; | |||
private List<Color> DefaultColors => _defaultColors ??= DefaultColorRanges.Analysis.ToList(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CreateParametersForColors() was already accepting List and didn't want to chage existing code unless I really have to. Hepe the latest commit is okay.
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
DefaultColorsNode
andDefaultIndicesNode
.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
*.resx
filesRelease 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