Fix #83322: Scale factor issue #105397

Loren Osborn · 2024-07-16T10:54:26+02:00

Loren Osborn commented

2024-07-16 10:54:26 +02:00

First-time contributor

Fix for issue #83322:

BoltFactory was written with 1mm Blender Units in mind (Blender
units are now officially 1m)
Addon already suppoted GLOBAL_SCALE constant, but assumed
inputs would always be in millimeters.
Since input fields now show units (that are incorrect here)
the input field units needed to be converted to Blender units first.
Then converted back to millimeters when constructing the mesh.
Then let the addon scale the result as it expected.

Fix for issue #83322: * BoltFactory was written with 1mm Blender Units in mind (Blender units are now officially 1m) * Addon already suppoted `GLOBAL_SCALE` constant, but assumed inputs would always be in millimeters. * Since input fields now show units (that are incorrect here) the input field units needed to be converted to Blender units first. * Then converted back to millimeters when constructing the mesh. * Then let the addon scale the result as it expected.

Loren Osborn added 1 commit 2024-07-16 10:54:29 +02:00

Fixed scale factor issue 44e358c60a

* BoltFactory was written with 1mm Blender Units in mind (Blender
    units are now officially 1m)
* Addon already suppoted `GLOBAL_SCALE` constant, but assumed
    inputs would always be in millimeters.
* Since input fields now show units (that are incorrect here)
    the input field units needed to be converted to Blender units first.
* Then converted back to millimeters when constructing the mesh.
* Then let the addon scale the result as it expected.

Iliya Katushenock changed title from ~~Fixed scale factor issue #83322~~ to Fix #83322: Scale factor issue

2024-07-16 10:56:29 +02:00

Loren Osborn referenced this pull request from extensions/add_mesh_BoltFactory

2024-07-16 20:32:13 +02:00

Fix #5: Scale factor issue #7

Nika Kutsniashvili referenced this issue from a commit

2024-07-17 13:24:01 +02:00

Fix #5: Scale factor issue (#7)

Loren Osborn commented

2024-07-17 19:49:38 +02:00

First-time contributor

@nickberckley suggested that this should be considered for #LTS36

(@brecht, ChatGPT seems to think that you’re Primary Person Responsible for 3.6 LTS maintenance, so I’m tagging you in this issue.)

@nickberckley suggested that this should be considered for #LTS36 (@brecht, ChatGPT seems to think that you’re Primary Person Responsible for 3.6 LTS maintenance, so I’m tagging you in this issue.)

Brecht Van Lommel commented

2024-07-24 21:58:46 +02:00

@lichtwerk is actually responsible for LTS maintenance. I guess he can decide if it's worth backporting, or get advice from another dev.

Philipp Oeser commented

2024-07-25 09:30:38 +02:00

Think it would be fine to backport this.
Would be good to mention that this also fixes #84782 ?

Will double check to procedure for the cases where we have a fix in the extension:

should a backport target main, be committed and then also go into blender-v3.6-release OR
should a backport target blender-v3.6-release directly?

Would assume the former would be the way to go @ThomasDinges ?
If so, this PR could be merged as is.
If the later is preferred, then this PR needs to target the blender-v3.6-release branch, check https://developer.blender.org/docs/handbook/contributing/pull_requests/#rebasing-a-pull-request on how to do this

In any case, it should be added to #109399

Think it would be fine to backport this. Would be good to mention that this also fixes #84782 ? Will double check to procedure for the cases where we have a fix in the extension: - should a backport target `main`, be committed and then also go into `blender-v3.6-release` OR - should a backport target `blender-v3.6-release` directly? Would assume the former would be the way to go @ThomasDinges ? If so, this PR could be merged as is. If the later is preferred, then this PR needs to target the `blender-v3.6-release` branch, check https://developer.blender.org/docs/handbook/contributing/pull_requests/#rebasing-a-pull-request on how to do this In any case, it should be added to #109399

Thomas Dinges commented

2024-07-25 10:08:15 +02:00

Fixes should always target main and then be backported to the LTS branch unless the fix is exclusively for the LTS branch.

Fixes should always target `main` and then be backported to the LTS branch unless the fix is exclusively for the LTS branch.

sw-tya reviewed 2024-07-30 01:25:50 +02:00

sw-tya left a comment

First-time contributor

The preset values that are stored in the folder /addons/presets/operator/mesh.bolt_add
are inputting values into the dialog box using mm rather than m. This means that the change presented here breaks the shipped presets.
All the distance values in the "m*.py" preset files need dividing by 1000.

The preset values that are stored in the folder /addons/presets/operator/mesh.bolt_add are inputting values into the dialog box using mm rather than m. This means that the change presented here breaks the shipped presets. All the distance values in the "m*.py" preset files need dividing by 1000.

👍 1

sw-tya commented

2024-07-30 01:39:44 +02:00

First-time contributor

Here are a couple of screen shots.
Initially looks OK:

Loading the m6 Operator Preset:

My edit of the preset file putting "/1000" on the end of most lines:

For reference the original output of the M6 preset as shipped with 4.1:

Here are a couple of screen shots. Initially looks OK: ![BoltFactory_default_mm_scale.png](/attachments/7c60356d-e9c4-49ae-8ee8-b178049d6068) Loading the m6 Operator Preset: ![BoltFactory_m6_mm_scale.png](/attachments/8a3e7e46-3914-4474-b3cb-a49092ac02a4) My edit of the preset file putting "/1000" on the end of most lines: ![BoltFactory_m6_preset_div1000.png](/attachments/af6b7b43-fd63-4ed1-a3e1-63398ce7d2c0) For reference the original output of the M6 preset as shipped with 4.1: ![BoltFactory_m6_1m_scale.png](/attachments/44c78028-18ca-4b37-a144-c346ceeff390)

BoltFactory_default_mm_scale.png

178 KiB

BoltFactory_m6_mm_scale.png

139 KiB

BoltFactory_m6_1m_scale.png

181 KiB

BoltFactory_m6_preset_div1000.png

167 KiB

sw-tya reviewed 2024-07-30 01:49:47 +02:00

add_mesh_BoltFactory/Boltfactory.py

						
				@ -28,3 +28,3 @@

				    MAX_INPUT_NUMBER = 50

				    MAX_INPUT_NUMBER = 50 # mm

sw-tya commented

2024-07-30 01:49:47 +02:00

First-time contributor

There was a ticket or comment saying that 50mm was too small for the maximal bolt size. The user wanted to make a bolt that was 2 inches which needed 50.8mm
If the scaling is now correctly working I don't see a need to have a limit in here. Practically 250 would be a better limit as that would cover anything that I have seen in agriculture.

There was a ticket or comment saying that 50mm was too small for the maximal bolt size. The user wanted to make a bolt that was 2 inches which needed 50.8mm If the scaling is now correctly working I don't see a need to have a limit in here. Practically 250 would be a better limit as that would cover anything that I have seen in agriculture.

Loren Osborn commented

2024-08-07 23:25:44 +02:00

First-time contributor

Practically 250 would be a better limit as that would cover anything that I have seen in agriculture.

Looking at the drivel on Wikipedia I got a bit bored and resorted to ChatGPT which said that specialty bolts can often exceed 4 inches (101.6mm)… which makes me think 250mm is probably overkill, but definitely sufficient, so I’ll make it 250mm

> Practically 250 would be a better limit as that would cover anything that I have seen in agriculture. Looking at the drivel on Wikipedia I got a bit bored and resorted to ChatGPT which said that specialty bolts can often exceed 4 inches (101.6mm)… which makes me think 250mm is probably overkill, but definitely sufficient, so I’ll make it 250mm

sw-tya commented

2024-08-08 16:43:57 +02:00

First-time contributor

https://www.globalfastener.com/standards/detail_4902.html
That shows a good range.
I hope that people who are in industry are using qualified models rather than open source representations!
Besides even with the limit is is possible to work round it by creating the single item, in a different project, at a reduced scale then importing.

https://www.globalfastener.com/standards/detail_4902.html That shows a good range. I hope that people who are in industry are using qualified models rather than open source representations! Besides even with the limit is is possible to work round it by creating the single item, in a different project, at a reduced scale then importing.

Loren Osborn commented

2024-08-09 19:41:54 +02:00

First-time contributor

This is a great reference, and I may actually use it in further enhancements I began working on.

I do think we should definitely support up to M160 or 160 mm, but I am hesitant to make this a hard limit until we see a similar reference for "standard"/imperial sizes.... until then we can leave it as the slightly ridiculous 250mm. Do you have a similar reference for imperial?

This is a great reference, and I may actually use it in further enhancements I began working on. I do think we should definitely support up to M160 or 160 mm, but I am hesitant to make this a hard limit until we see a similar reference for "standard"/imperial sizes.... until then we can leave it as the slightly ridiculous 250mm. Do you have a similar reference for imperial?

sw-tya commented

2024-08-10 00:08:35 +02:00

First-time contributor

This page covers imperial: https://www.globalfastener.com/standards/detail.php?sid=MjI4NTQ=
The "asme-b18.2.2" covers standard sizes, the 4 inch heavy nuts are 6 1/8 across flats. A 4 inch connecting nut is 9 1/8.
9.125 inch = 231mm, so 250mm is still a reasonable top end for the input box.
There are copies of this paid for standard accidentally published online which google has found.

This page covers imperial: https://www.globalfastener.com/standards/detail.php?sid=MjI4NTQ= The "asme-b18.2.2" covers standard sizes, the 4 inch heavy nuts are 6 1/8 across flats. A 4 inch connecting nut is 9 1/8. 9.125 inch = 231mm, so 250mm is still a reasonable top end for the input box. There are copies of this paid for standard accidentally published online which google has found.

👍 1

sw-tya reviewed 2024-08-05 23:30:21 +02:00

add_mesh_BoltFactory/createMesh.py

						
				@ -2398,2 +2396,2 @@

				                                                props.bf_Cap_Head_Dia * (1.0 / 19.0),

				                                                props.bf_Cap_Head_Dia * (1.0 / 19.0),

				                                                Bit_Dia, props.bf_Cap_Head_Dia/GLOBAL_SCALE,

				                                                props.bf_Shank_Dia, props.bf_Cap_Head_Height/GLOBAL_SCALE,

sw-tya commented

2024-08-05 23:30:21 +02:00

First-time contributor

The Shank_Dia has been missed for the GLOBAL_SCALE division.

Loren Osborn commented

2024-08-09 19:33:12 +02:00

First-time contributor

I searched for all props that already had division by GLOBAL_SCALE somewhere in the file with the following regex:

bf_(12_Point_(Head|Nut)_(Flange_Dia|Height)|Allen_Bit_Depth|Cap_Head_(Dia|Height)|Hex_((Head|Nut)_Flat_Distance|Nut_Height)|Pan_Head_Dia|Phillips_Bit_Depth|(CounterSink|Dome)_Head_Dia|Shank_(Dia|Length)|Thread_Length|Torx_Bit_Depth)(?!\s*/\s*GLOBAL_SCALE\b)

and found a second bf_Shank_Dia not divided by GLOBAL_SCALE at line 2419.

I verified no similar properties with this regex:

\bprops\s*\.\s*\w+(?<!_Percent)(?<!_Count)(?<!_Type)(?<!\.report(?=\())\b(?!\s*/\s*GLOBAL_SCALE\b)

I will fix these two instances of bf_Shank_Dia not divided by GLOBAL_SCALE.

Loren Osborn commented

2024-08-07 20:32:42 +02:00

First-time contributor

@lichtwerk, yes, I saw #84782, agreed it would be included in fix-set, but somehow let it fall on the floor. My mistake. Good catch.

Been a busy week here, possibly won’t get to review this for 48 hrs at soonest.

250mm sounds like it may be a bit small for mining and bridges… I will consult Wikipedia.

I will also review the issue around line 2396-2397 and make sure the PR is rebased to the correct branch.

Thank you for the feedback.

(I will also push any fixes to a new PR on the new repo)

@lichtwerk, yes, I saw #84782, agreed it would be included in fix-set, but somehow let it fall on the floor. My mistake. Good catch. Been a busy week here, possibly won’t get to review this for 48 hrs at soonest. 250mm sounds like it may be a bit small for mining and bridges… I will consult Wikipedia. I will also review the issue around line 2396-2397 and make sure the PR is rebased to the correct branch. Thank you for the feedback. (I will also push any fixes to a new PR on the new repo)

sw-tya commented

2024-08-08 16:53:03 +02:00

First-time contributor

Been a busy week here, possibly won’t get to review this for 48 hrs at soonest.
250mm sounds like it may be a bit small for mining and bridges… I will consult Wikipedia.
I will also review the issue around line 2396-2397 and make sure the PR is rebased to the correct branch.

(I will also push any fixes to a new PR on the new repo)

I'm happy to take the time to review changes here before they get onto the mainline.
Also I'm happy to play around and test 4.3 for the preset functionality in the new world of extensions.

> Been a busy week here, possibly won’t get to review this for 48 hrs at soonest. > 250mm sounds like it may be a bit small for mining and bridges… I will consult Wikipedia. > I will also review the issue around line 2396-2397 and make sure the PR is rebased to the correct branch. > > (I will also push any fixes to a new PR on the new repo) I'm happy to take the time to review changes here before they get onto the mainline. Also I'm happy to play around and test 4.3 for the preset functionality in the new world of extensions.

Loren Osborn commented

2024-08-09 19:58:02 +02:00

First-time contributor

The preset values that are stored in the folder /addons/presets/operator/mesh.bolt_add
are inputting values into the dialog box using mm rather than m. This means that the change presented here breaks the shipped presets.
All the distance values in the "m*.py" preset files need dividing by 1000.

I have yet to review this part yet, but (aside from the screenshot) it sounds like a good catch. (Can someone put more hours in a day please?)

> The preset values that are stored in the folder /addons/presets/operator/mesh.bolt_add > are inputting values into the dialog box using mm rather than m. This means that the change presented here breaks the shipped presets. > All the distance values in the "m*.py" preset files need dividing by 1000. I have yet to review this part yet, but (aside from the screenshot) it sounds like a good catch. (Can someone put more hours in a day please?)

Loren Osborn reviewed 2024-08-09 20:01:21 +02:00

add_mesh_BoltFactory/Boltfactory.py

						
				@ -288,42 +288,42 @@ class add_mesh_bolt(Operator, AddObjectHelper):

				            )

				    bf_Hex_Nut_Height: FloatProperty(

				            attr='bf_Hex_Nut_Height',

				            name='Hex Nut Height', default=2.4000000953674316,

Loren Osborn commented

2024-08-09 19:49:33 +02:00

First-time contributor

(Actually commenting on 268-281)

According to the info here on Wikipedia illustrated here, https://en.wikipedia.org/wiki/Unified_Thread_Standard#/media/File:ISO_and_UTS_Thread_Dimensions.svg , The default bf_Crest_Percent should be 12.5% and bf_Root_Percent should be 25% necessitating these be changed to FloatPropertys...

I had originally planned to make this part of a later PR, but I could include it here.

(Actually commenting on 268-281) According to the info here on Wikipedia illustrated here, https://en.wikipedia.org/wiki/Unified_Thread_Standard#/media/File:ISO_and_UTS_Thread_Dimensions.svg , The default `bf_Crest_Percent` should be 12.5% and `bf_Root_Percent` should be 25% necessitating these be changed to `FloatProperty`s... I had originally planned to make this part of a later PR, but I could include it here.

sw-tya commented

2024-08-10 00:37:26 +02:00

First-time contributor

I would hope that 0.5% is within the tolerance of the threading.

A bigger issue is the thread angle, the modelling today uses two diameters and two percentages for building the thread.
For both standard UN and metric the thread angle is 60 degrees, but there are other thread types of BA (47.5 deg), Whitworth (55 deg) and ACME (29 deg). The BA even has a rounded root profile.

When making it a float then the context menu box slider buttons will increment in 0.01 steps unless the GUI properties are also updated.
The global scale has changed the behaviour of the buttons already e.g. the diameter entry steps in 1cm jumps making it un-useable. I tried editing the FloatProperty with "precision" and/or "step" but it did not seem to do the right thing.

I would hope that 0.5% is within the tolerance of the threading. A bigger issue is the thread angle, the modelling today uses two diameters and two percentages for building the thread. For both standard UN and metric the thread angle is 60 degrees, but there are other thread types of BA (47.5 deg), Whitworth (55 deg) and ACME (29 deg). The BA even has a rounded root profile. When making it a float then the context menu box slider buttons will increment in 0.01 steps unless the GUI properties are also updated. The global scale has changed the behaviour of the buttons already e.g. the diameter entry steps in 1cm jumps making it un-useable. I tried editing the FloatProperty with "precision" and/or "step" but it did not seem to do the right thing.

Loren Osborn commented

2024-08-10 07:59:03 +02:00

First-time contributor

Modifying the inputs to include an angle as one of the inputs was part of a larger change I was working on, but turned out to be more involved, so I hadn't planned on backporting that part. I understand that the minor diameter inc/dec buttons are unfortunately useless currently. I don't know what can be easily done about this.

Suggestions welcome.

Modifying the inputs to include an angle as one of the inputs was part of a larger change I was working on, but turned out to be more involved, so I hadn't planned on backporting that part. I understand that the minor diameter inc/dec buttons are unfortunately useless currently. I don't know what can be easily done about this. Suggestions welcome.

This pull request can be merged automatically.

You are not authorized to merge this pull request.

View command line instructions.