DynamoDS / Dynamo

Open Source Graphical Programming for Design
https://dynamobim.org
Other
1.74k stars 634 forks source link

Continuity Error in Vectors #7707

Open Amoursol opened 7 years ago

Amoursol commented 7 years ago

If this issue is with Dynamo for Revit, please post your issue on the Dynamo for Revit Issues page.

If this issue is not a bug report or improvement request, please check the Dynamo forum, and start a thread there to discuss your issue.

Dynamo version

1.2.1.3083

Operating system

Windows 10

What did you do?

Tried to filter based off the X and Y Vector numbers. In the FamilyInstance.FacingOrientation node the outputs read as zero, but when I used a Vector.X/Y node, they came in as very small numbers instead (-3.24078139034181e-15)

What did you expect to see?

I expected there to be continuity between the nodes. Maybe some form of rounding if the number sits below a certain threshold is warranted?

What did you see instead?

There is a variance in output between the full Vector and the Query node of Vector.X / Vector.Y. This is showcased in the attached image. This also affects the Vector.Normalized node and most likely more.

dynamobim_weirdness

image

ke-yu commented 7 years ago

Yah, they are inconsistent, for geometry node the preview only keeps 3 digits after dot.

But, we expect geometry values are in some range (Setting | Geometry Scaling...), so the precision of normal nodes probably could be different from geometry nodes. @Racel any though?

Racel commented 7 years ago

@ke-yu - Do you mean that we keep only the 3 digits after the dot for nodes that output geometry? or for nodes that belong to any geometry class?

This is a tough call. In some ways, the very small number might indicate to the user that they do not in fact have things lined up correctly particularly for optimization workflows as @mjkkirschner mentions in #7727. Here are a couple of options to consider @mjkkirschner @aparajit-pratap @ke-yu :

I kind of prefer option 2 better for a few reasons:

Let me know what you think. Or if my assumptions are incorrect about the frequency of this issue or the ASM tolerance.

aparajit-pratap commented 7 years ago

@Racel firstly the "number format" option in the Settings menu only applies to number nodes so it doesn't look really useful to me personally. Can we get rid of it OR use it to set the format for all node previews universally?

If not, Option 2 looks good but we then should make it consistent for all nodes including geometry nodes, meaning geometry node previews should also display upto 6 decimal places, not the default 3.

pboyer commented 7 years ago

@Racel @aparajit-pratap As Jean Luc Picard would say, there's always another option:

This format is usually applied for string serialization of floating point numbers. In .NET it's known as the "round trip" format specifier: https://msdn.microsoft.com/en-us/library/dwhawy9k.aspx#RFormatString

Hence, for any floating point number, the user can look at the number and completely distinguish it from any other number.

I would argue that this should be the default and there should be no other option (KISS), except if someone passes the number through some sort of string formatting node, wherein they would have complete control.

@Racel Your option 2 is clever but there are some important caveats, but we can discuss it offline.

aparajit-pratap commented 7 years ago

@pboyer thanks for this information but we are trying to solve a slightly different problem of preventing to display highly precise values with lots of decimal places in the node previews and I think it makes sense to restrict this display to 6 decimal places (to the order of 10e-6) which is the highest precision we are dealing with in the geometry library or maybe round off to even 8 decimals for other workflows such as optimization?

ThomasMahon commented 7 years ago

@aparajit-pratap @ke-yu @Racel Related issue?

image

mjkkirschner commented 7 years ago

@Racel I think we should try to tackle this - I have always found this display setting confusing, and would as @aparajit-pratap refers above, prefer to get rid of this or try @pboyer 's solution.