DynamicData icon indicating copy to clipboard operation
DynamicData copied to clipboard

Units in configurations

Open sphh opened this issue 1 year ago • 10 comments

Many thanks for your workbench!

I just created my first configuration. As shown in your documentation, I want to make configurations for the variables ddHeight, ddLength, ddRadius, which are all of the type Length. However I cannot add the unit to the configurations, either during creation of the configuration or after by adding the <variable>List.

When I change the configuration by selecting another size, some formulae fail, e.g. dd.ddWidth = dd.ddHeight + 10mm because of different units. But I had to add the mm unit to 10 in that formula, because ddWidth is of type Length.

How do I add units to the configuration lists?

PS: You might also want to prepend the variables with the (default) dd-prefix to show, that this is needed here.

sphh avatar Dec 08 '23 14:12 sphh

I can't reproduce the issue. Maybe if I had your file to work with I would be able to. Try multiplying by 1mm in the cases where you need to convert a unitless value to a unit quantity and dividing by 1mm in the opposite cases.

mwganson avatar Dec 08 '23 18:12 mwganson

No problem:

  1. Here I created a body which uses the variables ddLength, ddHeight and ddRadius, all of the type Length. In the sketch there is also a full circle with the radius with the formula dd.ddRadius - 2mm. I had to use the unit mm here, so that the evaluation of this formula does not fail: ddBasis.FCStd.zip
  2. I then added a configuration for ddLength, ddHeight and ddRadius: ddConfiguration.FCStd.zip. This fails with the following error message:
22:15:28  Added property ddLengthList
22:15:28  Removed property ddLength
22:15:28  Added property ddLength
22:15:28  Added property ddHeightList
22:15:28  Removed property ddHeight
22:15:28  Added property ddHeight
22:15:28  Added property ddRadiusList
22:15:28  Removed property ddRadius
22:15:28  Added property ddRadius
22:15:28  <Exception> Quantity::operator -(): Unit mismatch in minus operation
in property binding 'ddConfiguration#Sketch.Constraints'
22:15:28  Recompute failed!

I could correct this, by changing the formula for the full circle in the sketch as you lined out. I find this somewhat counter intuitive, if I correctly selected the right variable type when creating the variable ddRadius in the first place.

It would also be nice to be able to mix the units in the configuration's variable lists, let's say mm and m

PS: This is a screenshot, if I do not prepend the variables with dd (to prove my point in the OP's PS): Screenshot from 2023-12-08 21-46-38

sphh avatar Dec 08 '23 21:12 sphh

The values in the tables have to be unitless because they're getting assigned to floating point list properties. FreeCAD only provides floatlist, integerlist, and stringlist for the list property types for general purpose values. There is no LengthList or AngleList, for example. There is a Vector List and a Material List, but those types are not suitable for our purposes here. So, we have to make do with the more general purpose float list.

I am moving away from requiring the dd prepending with newer features. It seemed like a good idea at the time, but most people don't like it. At some point I will probably go in and remove that requirement for all features. It'll be a bit of a chore because all of the old features are expecting that naming convention.

mwganson avatar Dec 09 '23 01:12 mwganson

Thanks for the explanation. I understand now, why it's not possible to add a unit to the value lists.

That is really a pity, because I strongly believe, that having values with units is the best way to reduce the number of errors related to dimensions …

I admit, that I don't know anything about the internals of FreeCAD or Python programming for it, but wouldn't it be possible to add a new property, let's say LengthUnit and then use this and the selected value from LengthList to assemble the Length property?

Or is it possible to convert a string property to a property with an unit attached? Just some ideas …

PS: I see, that the conversion from dd… to non-dd values is an ongoing process. I only would suggest to insert the dd… values as defaults in that configuration creation dialog until the transition is done successfully to help the user.

sphh avatar Dec 09 '23 15:12 sphh

PS: I wanted to look into the task to remove the dd prefix, so that I can make a PR. I then noticed, that this has been done already in: https://github.com/tri4ng1e/DynamicData/commit/47acffed2436a03697c6f0a36c285586d7d86466 (but that branch might not be up to date, because it is already a year old) …

sphh avatar Dec 09 '23 17:12 sphh

I never noticed it. You can test if you like. The copying and setting functions need to still work.

mwganson avatar Dec 10 '23 07:12 mwganson

I think it should be a preference, with the default to keep the current behavior. Then the user can disable it if desired or keep it he likes it.

mwganson avatar Dec 10 '23 07:12 mwganson

I had a look at it and my approach would be:

  1. Create an additional preference entry for the default prefix, let's say DefaultPropertyPrefix which defaults to 'dd'.
  2. When creating a new DynamicData object add a property owned by the object, let's say PropertyPrefix.
  3. Instead of using 'dd' as prefix in DynamicDataCmd.py, throughout use the property PropertyPrefix instead.

Which this approach, there is a PropertyPrefix property for each DynamicData object. The default value for this PropertyPrefix can be set in the global preference, but it can be changed by the user (it might be worth considering to change the prefix of all properties, if this prefix is changed).

I hope this is what you have in mind.

I have already an implementation of this approach in DynamicDataCmd.py, but would need help with adding the DefaultPropertyPrefix preference to the dynamicdataprefs.ui. Could you please make that part available to me, so that I can reuse it. At the moment I have the following as first item in <layout class="QVBoxLayout"> just before the <item><widget class="Gui::PrefCheckBox" name="KeepToolbar">:

   <item>
    <layout class="QHBoxLayout">
     <property name="spacing">
      <number>3</number>
     </property>
     <property name="leftMargin">
      <number>0</number>
     </property>
     <property name="topMargin">
      <number>0</number>
     </property>
     <property name="rightMargin">
      <number>0</number>
     </property>
     <property name="bottomMargin">
      <number>0</number>
     </property>
     <item>
      <widget class="QLabel" name="Label0">
       <property name="text">
        <string>Automatic prefix for all properties:</string>
       </property>
      </widget>
     </item>
     <item>
      <widget class="Gui::PrefLineEdit" name="DefaultPropertyPrefix">
???
      </widget>
     </item>
    </layout>
   </item>

sphh avatar Dec 10 '23 15:12 sphh

My apologies. I've changed my mind after thinking more on this. The dd prefix is no longer as useful as it was when the workbench was created. Back then you needed to enter at least 2 characters of the property name into the expression editor (such as when binding a constraint to a property) in order to get the suggestion list of available property names. In dev 0.22 (and maybe in 0.21 release) now you get all the properties of the object after entering the object name and the dot. For example, "dd." gives you all the properties of the dd object in a list to select from. That was not the case in earlier versions.

For that reason I've decided to just do away with the dd prefix entirely instead of a prefix preference. This should make it simpler to code, leading to fewer bugs and other issues. The copy/set command is the one that is most likely to break by removing the dd prefix, I think, but I really need to rewrite those, anyway, giving them a proper dialog instead of all the QInputDialogs currently being used. It might be possible to apply that old branch to the new code. I haven't looked into that yet. Or if you want to submit a PR, that's fine, too.

mwganson avatar Dec 10 '23 17:12 mwganson

In that case I would suggest the following:

  • Add the PropertyPrefix to the DynamicData object:
    • If the version is below 0.22, populate it with 'dd'
    • If the version is ≥0.22, leave it empty
  • Do not implement a global setting DefaultDynamicData

The advantage is, that this does not break in versions <0.22 but also uses the new feature of 0.22 and above. If the user wants to change the behaviour, s/he can always set the PropertyPrefix to his/her liking, e.g. to keep the old behaviour, because s/he is so used to type dd.dd….

Only disadvantage: A small increase of code complexity, but it is not much, because using f-strings make it easy again to apply the prefix.

What do you think?

BTW: That is working already in my branch. I had not looked into the coping/setting commands, but if you want to rewrite them anyway, I keep my fingers from them.

sphh avatar Dec 10 '23 17:12 sphh