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.

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

DYN-7392 Forge Units reference #15512

Merged
merged 5 commits into from
Nov 6, 2024
Merged

DYN-7392 Forge Units reference #15512

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
bb451bc
Forge Units reference
QilongTang Sep 26, 2024
50e67ae
Merge branch 'master' into ForgeUnits
pinzart90 Nov 4, 2024
9fd4076
update
pinzart90 Nov 4, 2024
6d77ec5
Merge branch 'master' into ForgeUnits
pinzart90 Nov 5, 2024
bff5e0f
update
pinzart90 Nov 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/DynamoCore/DynamoCore.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@
<PackageReference Include="Lucene.Net" Version="4.8.0-beta00016" />
<PackageReference Include="Lucene.Net.Analysis.Common" Version="4.8.0-beta00016" />
<PackageReference Include="Lucene.Net.QueryParser" Version="4.8.0-beta00016" />
<PackageReference Include="DynamoVisualProgramming.Analytics" Version="4.2.1.4086" />
<PackageReference Include="DynamoVisualProgramming.Analytics" Version="4.2.1.6685" />
</ItemGroup>
<ItemGroup>
<ProjectReference Include="..\Engine\GraphLayout\GraphLayout.csproj">
Expand Down
4 changes: 2 additions & 2 deletions src/Libraries/DynamoUnits/UnitsCore.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,11 @@
<ItemGroup>
<PackageReference Include="System.Configuration.ConfigurationManager" Version="5.0.0" />
<PackageReference Include="System.Resources.Extensions" Version="5.0.0" />
<PackageReference Include="ForgeUnits.NET" Version="5.0.5" GeneratePathProperty="true">
<PackageReference Include="ForgeUnits.NET" Version="5.3.2" GeneratePathProperty="true">
<!--Skip copying the runtime dlls because we only need one (ForgeUnitsManaged.dll) which we copy in a task bellow.-->
<ExcludeAssets>runtime;build</ExcludeAssets>
</PackageReference>
<PackageReference Include="ForgeUnits.Schemas" Version="1.0.1" GeneratePathProperty="true" />
<PackageReference Include="ForgeUnits.Schemas" Version="1.0.4" GeneratePathProperty="true" />
Copy link
Member

Choose a reason for hiding this comment

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

so does this work in Revit?

Copy link
Contributor

Choose a reason for hiding this comment

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

The compatibility discussion is here:
https://autodesk.slack.com/archives/CA4SXCD4L/p1730755756302839?thread_ts=1730726649.001799&cid=CA4SXCD4L

</ItemGroup>
<ItemGroup>
<Content Include="DynamoUnits_DynamoCustomization.xml" />
Expand Down
2 changes: 1 addition & 1 deletion test/DynamoCoreTests/DynamoCoreTests.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@
<PackageReference Include="coverlet.collector" Version="3.1.2" />
<PackageReference Include="Microsoft.NET.Test.Sdk" Version="17.3.2" ExcludeAssets="none" />
<PackageReference Include="JunitXml.TestLogger" Version="3.0.124" />
<PackageReference Include="DynamoVisualProgramming.Analytics" Version="4.2.1.4086">
<PackageReference Include="DynamoVisualProgramming.Analytics" Version="4.2.1.6685">
<PrivateAssets>all</PrivateAssets>
<IncludeAssets>compile; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets>
</PackageReference>
Expand Down
2 changes: 1 addition & 1 deletion test/DynamoCoreTests/UnitsOfMeasureTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -962,7 +962,7 @@ public void CanCreateForgeQuantity_FromFutureTypeId()
public void ForgeQuantityContainsMultipleUnits()
{
var quantityType = Quantity.ByTypeID("autodesk.unit.quantity:length-1.0.5");
Assert.GreaterOrEqual(16,quantityType.Units.Count());
Assert.GreaterOrEqual(17,quantityType.Units.Count());
}
[Test, Category("UnitTests")]
public void ForgeQuantityEquality()
Expand Down
2 changes: 1 addition & 1 deletion test/DynamoCoreWpfTests/UnitsUITests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,7 +199,7 @@ public void ForgeUnitDropdownsLoadWithMalformedData()
//we've changed both index and name, so a new item is selected.
Assert.AreNotEqual(DynamoUnits.Quantity.ByTypeID("autodesk.unit.quantity:force-1.0.2"), node3.CachedValue.Data);
Assert.AreEqual(DynamoUnits.Symbol.ByTypeID("autodesk.unit.symbol:mm-1.0.1"), node4.CachedValue.Data);
Assert.AreEqual(DynamoUnits.Unit.ByTypeID("autodesk.unit.unit:millimeters-1.0.1"), node5.CachedValue.Data);
Assert.AreEqual(DynamoUnits.Unit.ByTypeID("autodesk.unit.unit:microarcseconds-1.0.1"), node5.CachedValue.Data);
Copy link
Member

Choose a reason for hiding this comment

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

?

Copy link
Contributor

Choose a reason for hiding this comment

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

new unit changes went in so the order changed
https://git.autodesk.com/forge/HFDMSchemas/pull/148/files#diff-d5a66c435e446495cd9f736484e9a5bbcffb1e9fb15e7542418fadc479f2ad19

Copy link
Member

Choose a reason for hiding this comment

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

It seems to me this test is trying to assert exactly what you saying is NOT happening.
IE when new schema data is introduced the nodes should not just return new values... that will break graphs.

Copy link
Member

Choose a reason for hiding this comment

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

@QilongTang @saintentropy do I have that right? I kind of remember this is a general problem with dropdowns on the Revit side, though I thought we had semi fixed it with string matching as well as index matching at some point?
I am remembering incorrectly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, to me it looks like opening a legacy graph the node is returning a different value, if the old value is still there from the list, then this could be a bug worth fixing. I would suggest checking the serialized index and node value and compare with the new graph opening

Copy link
Contributor

@pinzart90 pinzart90 Nov 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The node in question

      "SelectedIndex": 221,
      "SelectedString": "Millimeters AAA",
      "NodeType": "ExtensionNode",
      "Id": "2cc49d25d3b447b281a81efd6173c6cf",
      ```
It has an intentional malformed selection "Millimeters AAA" - that does not exist.
So in turn it defaults to the selected index (221) which now is different (points to microarcsecond), because of the changes that went in

Copy link
Member

@mjkkirschner mjkkirschner Nov 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I don't want to hold this up further, but I am trying to understand the following:

  1. Is this failure an existing issue with all dropdowns or just these unit nodes?
  2. What was this test intending to test? Does it still test that? Is it valuable?

Copy link
Contributor

@pinzart90 pinzart90 Nov 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

  1. I think this scenario could affect all dropdowns
  2. It looks to me like this test wants to cover the stability of unit drop downs. When the serialized unit selection exists at runtime and when it does not exist anymore. I think there is still value in it
    If a unit value is renamed, the selectedIndex could still point to the new (renamed) unit.
    This test is prone to failures if new units are added or removed.

Assert.AreEqual(DynamoUnits.Unit.ByTypeID("autodesk.unit.unit:meters-1.0.1"), node6.CachedValue.Data);
Assert.AreEqual(DynamoUnits.Unit.ByTypeID("autodesk.unit.unit:millimeters-1.0.1"), node7.CachedValue.Data);
Assert.AreEqual(3, node8.CachedValue.GetElements().Count());
Expand Down
Loading