Add feature #12: Scale geometry options for shape import #16

Merged
Cedric Steiert merged 3 commits from Hombre57/io_scene_x3d:import-scale into main 2024-09-10 00:49:23 +02:00
Collaborator

The geometry data is recomputed (positions, size, transformations),
without modifying the transformation matrix.

The geometry data is recomputed (positions, size, transformations), without modifying the transformation matrix.
Hombre57 added the
enhancement
label 2024-09-08 00:14:46 +02:00
Hombre57 added 1 commit 2024-09-08 00:14:47 +02:00
The geometry data is recomputed (positions, size, transformations),
without modifying the transformation matrix.
Cedric Steiert requested changes 2024-09-08 18:32:02 +02:00
Dismissed
Cedric Steiert left a comment
Collaborator

I'm quite satisifed with what you've already got here. 👍
Would be great if you could go through the comments and resolve or discuss them.

One thing i stumbled across: scaling does not work when using a primitive like box. I don't hink this was working before either but would be great if it would, especially now with the unit selector. I attached a simple test file, changing the unit does not result in the box being smaller. Is it possible to incllude such objects as well or is the implementation effort too high? If it's not possible please document it soemwhere, that scaling is only available with custom objects (indexed face set).

I'm quite satisifed with what you've already got here. 👍 Would be great if you could go through the comments and resolve or discuss them. One thing i stumbled across: scaling does not work when using a primitive like box. I don't hink this was working before either but would be great if it would, especially now with the unit selector. I attached a simple test file, changing the unit does not result in the box being smaller. Is it possible to incllude such objects as well or is the implementation effort too high? If it's not possible please document it soemwhere, that scaling is only available with custom objects (indexed face set).
@ -39,1 +39,4 @@
file_unit: EnumProperty(
name="File Unit",
items=(('M', "Meter", "The VRML/x3D specification states that data have to be exported in Meter."),
Collaborator

three improvement suggestions:

  1. No dot at the end of the sentence. A dot will be added automatically by blender (thus currently resulting in "..").
  2. Make the description more user-friendly, sth like "Meter: the default unit of vrml (wrl) files. Most programs assume the objects to be defined in meters as per specification."
  3. include scale factor (in case users prefer to set it manually)
three improvement suggestions: 1. No dot at the end of the sentence. A dot will be added automatically by blender (thus currently resulting in ".."). 2. Make the description more user-friendly, sth like "Meter: the default unit of vrml (wrl) files. Most programs assume the objects to be defined in meters as per specification." 4. include scale factor (in case users prefer to set it manually)
Author
Collaborator
  1. & 2. Done
  2. no need to include the scale in tooltip, see below
1. & 2. Done 3. no need to include the scale in tooltip, see below
Bujus_Krachus marked this conversation as resolved
@ -40,0 +40,4 @@
file_unit: EnumProperty(
name="File Unit",
items=(('M', "Meter", "The VRML/x3D specification states that data have to be exported in Meter."),
('DM', "Decimeter", ""),
Collaborator

Here are the descriptions missing. Two points that could be included:

  • what programs use that unit scale
  • include the scale factor
Here are the descriptions missing. Two points that could be included: - what programs use that unit scale - include the scale factor
Author
Collaborator

I don't know if any software will use that scale, I don't think anyone will, but I added in the list all units between meter and millimeter. I hesitated to add the intermediate units... If you want a specific list of entries, just drop them in a comment below.

I'll add the scale in the comments for each entries.

I don't know if any software will use that scale, I don't think anyone will, but I added in the list all units between meter and millimeter. I hesitated to add the intermediate units... If you want a specific list of entries, just drop them in a comment below. I'll add the scale in the comments for each entries.
Bujus_Krachus marked this conversation as resolved
@ -40,0 +41,4 @@
name="File Unit",
items=(('M', "Meter", "The VRML/x3D specification states that data have to be exported in Meter."),
('DM', "Decimeter", ""),
('CM', "Centimeter", ""),
Collaborator

description, see above

description, see above
Hombre57 marked this conversation as resolved
@ -40,0 +42,4 @@
items=(('M', "Meter", "The VRML/x3D specification states that data have to be exported in Meter."),
('DM', "Decimeter", ""),
('CM', "Centimeter", ""),
('MM', "Milimeter", ""),
Collaborator

description, see above

description, see above
Hombre57 marked this conversation as resolved
@ -40,0 +43,4 @@
('DM', "Decimeter", ""),
('CM', "Centimeter", ""),
('MM', "Milimeter", ""),
('IN', "Inch", ""),
Collaborator

description, see above

description, see above
Hombre57 marked this conversation as resolved
@ -40,0 +44,4 @@
('CM', "Centimeter", ""),
('MM', "Milimeter", ""),
('IN', "Inch", ""),
('CUSTOM', "Use scale", "Prefer scaling the geometry with the scale factor below"),
Collaborator
  • Instead of "Use scale" use "CUSTOM"
  • Instead of "Prefer scaling the geometry with the scale factor below" maybe rewrite to "Use the scale factor provided below"
- Instead of "Use scale" use "CUSTOM" - Instead of "Prefer scaling the geometry with the scale factor below" maybe rewrite to "Use the scale factor provided below"
Author
Collaborator

Done

Done
Hombre57 marked this conversation as resolved
@ -249,2 +269,4 @@
operator = sfile.active_operator
layout.prop(operator, "file_unit")
layout.prop(operator, "global_scale")
Collaborator

I would prefer the scale input to be disabled until entry CUSTOM is selected (to avoid confusion for users in a rush), this could maybe be achieved by this solution: https://blender.stackexchange.com/a/268840

Also on another note a small research question: is it possible to dynamically change the value of the scale field depending on the selected dropdown entry? Idk if this would be helpful for some to understand what each scale option means or if it just clutters the importer.

I would prefer the scale input to be disabled until entry CUSTOM is selected (to avoid confusion for users in a rush), this could maybe be achieved by this solution: https://blender.stackexchange.com/a/268840 Also on another note a small research question: is it possible to dynamically change the value of the scale field depending on the selected dropdown entry? Idk if this would be helpful for some to understand what each scale option means or if it just clutters the importer.
Author
Collaborator

Widget sensitivity and update done. no need for scale in tooltips then.

Widget sensitivity and update done. no need for scale in tooltips then.
Bujus_Krachus marked this conversation as resolved
@ -15,6 +15,7 @@ from itertools import chain
texture_cache = {}
material_cache = {}
conversionScale = 1.0
Collaborator

use snake_case

use snake_case
Bujus_Krachus marked this conversation as resolved
@ -682,3 +683,3 @@
return default
def getFieldAsFloat(self, field, default, ancestry):
def getFieldAsFloat(self, field, default, ancestry, s=1.0):
Collaborator

nitpick - be more descriptive: what does "s" mean? I sit speed, symmetry, similarity, simplicity, maybe even scale?

nitpick - be more descriptive: what does "s" mean? I sit speed, symmetry, similarity, simplicity, maybe even scale?
Bujus_Krachus marked this conversation as resolved
@ -701,3 +702,3 @@
return default
def getFieldAsFloatTuple(self, field, default, ancestry):
def getFieldAsFloatTuple(self, field, default, ancestry, s=1.0):
Collaborator

nitpick - be more descriptive: what does "s" mean? I sit speed, symmetry, similarity, simplicity, maybe even scale?

nitpick - be more descriptive: what does "s" mean? I sit speed, symmetry, similarity, simplicity, maybe even scale?
Bujus_Krachus marked this conversation as resolved
@ -3597,2 +3610,4 @@
# Used when adding blender primitives
GLOBALS['CIRCLE_DETAIL'] = PREF_CIRCLE_DIV
global conversionScale
Collaborator

use snake_case

use snake_case
Bujus_Krachus marked this conversation as resolved
@ -3599,0 +3612,4 @@
global conversionScale
units_factor = {'M': 1.0, 'DM': 0.1, 'CM': 0.01, 'MM': 0.001, 'IN': 0.0254}
Collaborator

uppercase for readability as it seems to be a constant.

uppercase for readability as it seems to be a constant.
Author
Collaborator

Done

Done
Hombre57 marked this conversation as resolved
Cedric Steiert reviewed 2024-09-08 18:43:41 +02:00
@ -40,0 +53,4 @@
global_scale: FloatProperty(
name="Scale",
min=0.001, max=1000.0,
default=1.0,
Collaborator

please also add step=1.0,. Currently it steps in intervals of 0.03, 0.01 seems more appropriate (or is 0.03 more comfy?). Could be also adjusted for the x3d exporter in line 220.

please also add `step=1.0,`. Currently it steps in intervals of 0.03, 0.01 seems more appropriate (or is 0.03 more comfy?). Could be also adjusted for the x3d exporter in line 220.
Author
Collaborator

Done

Done
Hombre57 marked this conversation as resolved
Hombre57 added 1 commit 2024-09-09 01:07:27 +02:00
- tooltips updated
- Scale widget now only sensitive when Unit is set to CUSTOM
- Unit's scale is reflected in the Scale widget
- default values of primitive are now scaled too
Author
Collaborator

@Bujus_Krachus

One thing i stumbled across: scaling does not work when using a primitive like box.

It was already working for standard cases where the box size is provided in the file. The default values were not scaled, but they are now.

@Bujus_Krachus > One thing i stumbled across: scaling does not work when using a primitive like box. It was already working for standard cases where the box size is provided in the file. The default values were not scaled, but they are now.
Cedric Steiert approved these changes 2024-09-09 02:01:16 +02:00
Dismissed
Cedric Steiert left a comment
Collaborator

Nicely done 👍

Also big thanks that my little box now works as expected and for the dynamic default values in the scale input field :)

One small naming change (see comment) would be required, but then up until now ready to get added as new feature. If you think you're finished just remove the WIP.

Nicely done 👍 Also big thanks that my little box now works as expected and for the dynamic default values in the scale input field :) One small naming change (see comment) would be required, but then up until now ready to get added as new feature. If you think you're finished just remove the WIP.
@ -40,0 +56,4 @@
default=1.0,
precision=4,
step=1.0,
description="Scale value used when 'File Unit' is set to 'Use scale'",
Collaborator

'Use scale' now needs to be adjusted to the new name (CUSTOM)

'Use scale' now needs to be adjusted to the new name (CUSTOM)
Bujus_Krachus marked this conversation as resolved
Cedric Steiert changed title from WIP: Fix issue #12: Scale geometry when importing shapes to WIP: Add feature #12: Scale geometry options for shape import 2024-09-09 02:05:00 +02:00
Cedric Steiert requested changes 2024-09-09 02:13:55 +02:00
Dismissed
Cedric Steiert left a comment
Collaborator

Unfortunately i just discovered that now x3d import is broken. The attached sample file got exported by blender and is the default cube. Import now throws this error:

UnboundLocalError: cannot access local variable 'apply_scale' where it is not associated with a value

That should get fixed

Unfortunately i just discovered that now x3d import is broken. The attached sample file got exported by blender and is the default cube. Import now throws this error: `UnboundLocalError: cannot access local variable 'apply_scale' where it is not associated with a value` That should get fixed
Hombre57 added 1 commit 2024-09-09 22:51:59 +02:00
- Fixed tooltip string
- importMesh_ReadVertices now also scale geometry (used in other nodes)
- Scale in transforms was erroneously scaled
- scaling bugfix in getFieldAsArray
Hombre57 changed title from WIP: Add feature #12: Scale geometry options for shape import to Add feature #12: Scale geometry options for shape import 2024-09-09 22:52:28 +02:00
Author
Collaborator

Thanks for the sample file @Bujus_Krachus . I hope everything is fixed now.

Thanks for the sample file @Bujus_Krachus . I hope everything is fixed now.
Cedric Steiert approved these changes 2024-09-10 00:44:39 +02:00
Cedric Steiert left a comment
Collaborator

Seems good to me, thanks for the contribution :)

Translations as mentioned in #12 (comment) can be done later, still a bit undecided which languages to include, for further info see #17.

It would be great to have the unit type selctor also available for the x3d exporter, see #18. If you have some time left i'd be glad to see that added as well for the next release.

Will merge to master and publish the new version when either enough changes accumulated or at latest shortly after the upcoming blender 4.3 release in november.

Seems good to me, thanks for the contribution :) Translations as mentioned in https://projects.blender.org/extensions/io_scene_x3d/issues/12#issuecomment-1288451 can be done later, still a bit undecided which languages to include, for further info see https://projects.blender.org/extensions/io_scene_x3d/issues/17. It would be great to have the unit type selctor also available for the x3d exporter, see https://projects.blender.org/extensions/io_scene_x3d/issues/18. If you have some time left i'd be glad to see that added as well for the next release. Will merge to master and publish the new version when either enough changes accumulated or at latest shortly after the upcoming blender 4.3 release in november.
Cedric Steiert merged commit 7e0b0e903a into main 2024-09-10 00:49:23 +02:00
Sign in to join this conversation.
No description provided.