extensions/io_scene_x3d
1
1
Fork 3
Code Issues 9 Pull Requests Projects Releases Wiki Activity

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
Conversation 2 Commits 3 Files Changed 2 +77 -26
Hombre57 commented 2024-09-08 00:14:46 +02:00
Collaborator
Copy Link

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
Copy Link

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).
box.wrl
48 B
source/__init__.py Outdated
@ -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."),
Cedric Steiert commented 2024-09-08 17:57:52 +02:00
Collaborator
Copy Link

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)
Hombre57 commented 2024-09-09 00:07:21 +02:00
Author
Collaborator
Copy Link
  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
source/__init__.py
@ -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", ""),
Cedric Steiert commented 2024-09-08 17:59:28 +02:00
Collaborator
Copy Link

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
Hombre57 commented 2024-09-08 23:06:04 +02:00
Author
Collaborator
Copy Link

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
source/__init__.py
@ -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", ""),
Cedric Steiert commented 2024-09-08 18:00:27 +02:00
Collaborator
Copy Link

description, see above

description, see above
Hombre57 marked this conversation as resolved
source/__init__.py
@ -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", ""),
Cedric Steiert commented 2024-09-08 18:00:35 +02:00
Collaborator
Copy Link

description, see above

description, see above
Hombre57 marked this conversation as resolved
source/__init__.py
@ -40,0 +43,4 @@
('DM', "Decimeter", ""),
('CM', "Centimeter", ""),
('MM', "Milimeter", ""),
('IN', "Inch", ""),
Cedric Steiert commented 2024-09-08 18:00:39 +02:00
Collaborator
Copy Link

description, see above

description, see above
Hombre57 marked this conversation as resolved
source/__init__.py Outdated
@ -40,0 +44,4 @@
('CM', "Centimeter", ""),
('MM', "Milimeter", ""),
('IN', "Inch", ""),
('CUSTOM', "Use scale", "Prefer scaling the geometry with the scale factor below"),
Cedric Steiert commented 2024-09-08 18:02:29 +02:00
Collaborator
Copy Link
  • 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"
Hombre57 commented 2024-09-08 23:57:49 +02:00
Author
Collaborator
Copy Link

Done

Done
Hombre57 marked this conversation as resolved
source/__init__.py Outdated
@ -249,2 +269,4 @@
operator = sfile.active_operator
layout.prop(operator, "file_unit")
layout.prop(operator, "global_scale")
Cedric Steiert commented 2024-09-08 18:11:22 +02:00
Collaborator
Copy Link

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.
Hombre57 commented 2024-09-08 23:55:53 +02:00
Author
Collaborator
Copy Link

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
source/import_x3d.py Outdated
@ -15,6 +15,7 @@ from itertools import chain
texture_cache = {}
material_cache = {}
conversionScale = 1.0
Cedric Steiert commented 2024-09-08 18:23:42 +02:00
Collaborator
Copy Link

use snake_case

use snake_case
Bujus_Krachus marked this conversation as resolved
source/import_x3d.py Outdated
@ -682,3 +683,3 @@
return default
def getFieldAsFloat(self, field, default, ancestry):
def getFieldAsFloat(self, field, default, ancestry, s=1.0):
Cedric Steiert commented 2024-09-08 18:15:11 +02:00
Collaborator
Copy Link

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
source/import_x3d.py Outdated
@ -701,3 +702,3 @@
return default
def getFieldAsFloatTuple(self, field, default, ancestry):
def getFieldAsFloatTuple(self, field, default, ancestry, s=1.0):
Cedric Steiert commented 2024-09-08 18:15:26 +02:00
Collaborator
Copy Link

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
source/import_x3d.py Outdated
@ -3597,2 +3610,4 @@
# Used when adding blender primitives
GLOBALS['CIRCLE_DETAIL'] = PREF_CIRCLE_DIV
global conversionScale
Cedric Steiert commented 2024-09-08 18:25:32 +02:00
Collaborator
Copy Link

use snake_case

use snake_case
Bujus_Krachus marked this conversation as resolved
source/import_x3d.py Outdated
@ -3599,0 +3612,4 @@
global conversionScale
units_factor = {'M': 1.0, 'DM': 0.1, 'CM': 0.01, 'MM': 0.001, 'IN': 0.0254}
Cedric Steiert commented 2024-09-08 18:22:11 +02:00
Collaborator
Copy Link

uppercase for readability as it seems to be a constant.

uppercase for readability as it seems to be a constant.
Hombre57 commented 2024-09-08 23:57:17 +02:00
Author
Collaborator
Copy Link

Done

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

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.
Hombre57 commented 2024-09-08 23:56:43 +02:00
Author
Collaborator
Copy Link

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
Hombre57 commented 2024-09-09 01:07:48 +02:00
Author
Collaborator
Copy Link

@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
Copy Link

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.
source/__init__.py Outdated
@ -40,0 +56,4 @@
default=1.0,
precision=4,
step=1.0,
description="Scale value used when 'File Unit' is set to 'Use scale'",
Cedric Steiert commented 2024-09-09 01:56:12 +02:00
Collaborator
Copy Link

'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
Copy Link

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
testcube.x3d
2.3 KiB
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
Hombre57 commented 2024-09-09 22:54:29 +02:00
Author
Collaborator
Copy Link

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
Copy Link

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.
Reviewers
No reviewers
Cedric Steiert
Labels
Clear labels   
bug

Something is not working

  
confirmed

The issue is confirmed.

  
duplicate

This issue or pull request already exists

  
enhancement

New feature

  
help wanted

Need some help

  
High

Priority high: This issue should be worked on soon, as it breaks things drastically

  
known issue

A known technical limitation. It may get fixed eventually, but no support expected.

  
Normal

Priority normal: Not a high issue, still relevant

  
not a task

just some content

  
question

More information is needed

  
task

Tasks that should be worked on

  
wontfix

This won't be fixed

No Label
bug
confirmed
duplicate
enhancement
help wanted
High
known issue
Normal
not a task
question
task
wontfix
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: extensions/io_scene_x3d#16
No description provided.
Delete Branch "Hombre57/io_scene_x3d:import-scale"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?