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

DYN-6840: add new boundary condition node #15270

Merged
merged 17 commits into from
Jun 3, 2024
Merged

Conversation

pinzart90
Copy link
Contributor

@pinzart90 pinzart90 commented May 30, 2024

Purpose

https://jira.autodesk.com/browse/DYN-6840

Add new boundary condition node for panel surfaces

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
  • This PR contains no files larger than 50 MB

Release Notes

Add new boundary condition dropdown node for panel surfaces.

Reviewers

(FILL ME IN) Reviewer 1 (If possible, assign the Reviewer for the PR)

(FILL ME IN, optional) Any additional notes to reviewers or testers.

FYIs

(FILL ME IN, Optional) Names of anyone else you wish to be notified of

@pinzart90
Copy link
Contributor Author

Still have to add resources (although I am not sure if we want more localization for 3.2)

@github-actions github-actions bot changed the title add new boundary condition node DYN-6840: add new boundary condition node May 30, 2024
Copy link

@github-actions github-actions bot left a 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-6840

@pinzart90 pinzart90 requested a review from aparajit-pratap May 30, 2024 17:11
@pinzart90
Copy link
Contributor Author

image

Copy link

github-actions bot commented May 30, 2024

UI Smoke Tests

Test: success. 2 passed, 0 failed.
TestComplete Test Result
Workflow Run: UI Smoke Tests
Check: UI Smoke Tests - net8.0

namespace GeometryUIWpf
{
[IsVisibleInDynamoLibrary(true)]
[NodeName("PanelSurfaceBoundaryConditions")]
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we just make this singular like the enum itself - PanelSurfaceBoundaryCondition?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

PanelSurfaceBoundaryCondition will collide with the ENum coming from ProtoGeom (at least in the library view)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@aparajit-pratap is the layout spec good enough for now ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you move the other PanelSurfaceBoundaryCondition node under this new BoundaryConditioncategory as well? I think more than one node can have the same name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

or maybe there's a special setting to allow same name nodes under the same parent ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or I can change the name of the new node (again)

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe only the same named nodes are allowed if and only if they have different inputs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Amoursol what do you think about the layout above ? 2 entries with the same name "PanelSurfaceBoundaryCOndition"
ONe is a dropdown node, the other is category (I think) for individual boundaryCOndition nodes

Copy link
Contributor

@Amoursol Amoursol Jun 3, 2024

Choose a reason for hiding this comment

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

@pinzart90 As it's still experimental, I don't think layoutspec changes matter too much. So we can do any of those changes and evaluate later if need be.

On the name front, could we please name it "Select Boundary Condition"? This is similar to some Revit paradigms. Do we also need to mark it an Action, not a Query?

image

Probably not plural - unlike image. Sorry on that miss!

@pinzart90 pinzart90 marked this pull request as ready for review May 31, 2024 14:57
@pinzart90 pinzart90 requested a review from mjkkirschner May 31, 2024 14:57
[OutPortTypes(nameof(PanelSurfaceBoundaryCondition))]
[OutPortDescriptions("PanelSurface BoundaryCondition enum value")]
[IsDesignScriptCompatible]
public class PanelSurfaceBoundaryConditionDropDown : DSDropDownBase
Copy link
Member

@mjkkirschner mjkkirschner May 31, 2024

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is also an EnumBase class inside Dynamo, but it generates the string values not the actual enum types.

public abstract class EnumBase<T> : DSDropDownBase

I guess they could all be made into a more generic class. I would consolidate them in another task

/// </summary>
public override IEnumerable<AssociativeNode> BuildOutputAst(List<AssociativeNode> inputAstNodes)
{
//Some of the legacy categories which were not working before will now be out of index.
Copy link
Contributor

Choose a reason for hiding this comment

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

What does this comment mean?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nothing, I can remove it

@@ -164,7 +164,7 @@ public void ProducesCorrectOutputFromCoreDirectory_preloadedbinaries()
FromDirectoryCommand.HandleDocumentationFromDirectory(opts);

var generatedFileNames = tempDirectory.GetFiles().Select(x => x.Name);
Assert.AreEqual(707, generatedFileNames.Count());
Assert.AreEqual(705, generatedFileNames.Count());
Copy link
Contributor

Choose a reason for hiding this comment

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

I just changed this to 709??

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll pull your changes and update this

@pinzart90 pinzart90 merged commit de384e7 into master Jun 3, 2024
23 of 24 checks passed
@pinzart90 pinzart90 deleted the add_boundarycond_enum_node branch June 3, 2024 20:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants