Fix per vertex normals and colors import #6

Hombre57 · 2024-07-21T11:05:05+02:00

Hombre57 commented

2024-07-21 11:05:05 +02:00

The 2 attached files can't be loaded properly with per-vertex normals and per-vertex colors. The proposed PR tries to update the code to the actual Blender API. However it might be a partial fix, I'm not used to. The change is inspired from other importer, which explains that the validation process might reorder the vertex list (and I experienced that...). That's why we have to save the normals and colors data to vertices before requiring validation, then move the data to their correct attributes.

The per-vertex color fix also add the shader node now and works fine here. I don't know if it has side effects on other files though.

The 2 attached files can't be loaded properly with per-vertex normals and per-vertex colors. The proposed PR tries to update the code to the actual Blender API. However it might be a partial fix, I'm not used to. The change is inspired from other importer, which explains that the validation process might reorder the vertex list (and I experienced that...). That's why we have to save the normals and colors data to vertices before requiring validation, then move the data to their correct attributes. The per-vertex color fix also add the shader node now and works fine here. I don't know if it has side effects on other files though.

normals.wrl

1.4 MiB

colors.wrl

107 KiB

Hombre57 added 1 commit 2024-07-21 11:05:07 +02:00

Fix per vertex normals and colors import fd2106afa5

Cedric Steiert commented

2024-07-21 13:16:33 +02:00

Could you maybe include the issue numbers or error messages/misbehaviours the PR fixes? Just for documentation purposes...

Hombre57 commented

2024-07-21 13:55:15 +02:00

There was no opened ticket for that, should I've had open one first ?

Cedric Steiert commented

2024-07-21 13:56:45 +02:00

There was no opened ticket for that, should I've had open one first ?

Nah, that's okay. Just include some details into the PR message or as comment. Ideally you can tell what get's fixed without opening a file :)

> There was no opened ticket for that, should I've had open one first ? Nah, that's okay. Just include some details into the PR message or as comment. Ideally you can tell what get's fixed without opening a file :)

Hombre57 commented

2024-07-21 17:03:05 +02:00

Ok, then for the record, the normals.wrl file is issued by TopSolid V6 with normals per vertex. colors.wrl is a truncated file from another ticket I can't get my hand on anymore.

A : File normals loaded with the unpatched code. It comes in Flat shading by default
B : The you activate Smooth shading and see that normals are completly off
C : File normals loaded with the patched code. It comes in Smooth shading by default with correct normals
D : File colors loaded with the unpatched code. The Color Attribute node has been added manually. The colors seems identical to the patched code, but using the same technique the prevent the validation to mess things up is required, IMHO.
E : File colors loaded with the patched code, the Color Attribute node has been automatically set.

Adding WIP status because this code probably needs an update of the required Blender version in __init__.py, but I don't know which one exactly. This patch works with 4.1.

Ok, then for the record, the `normals.wrl` file is issued by TopSolid V6 with normals per vertex. `colors.wrl` is a truncated file from another ticket I can't get my hand on anymore. - A : File `normals` loaded with the unpatched code. It comes in Flat shading by default - B : The you activate Smooth shading and see that normals are completly off - C : File `normals` loaded with the patched code. It comes in Smooth shading by default with correct normals - D : File `colors` loaded with the unpatched code. The Color Attribute node has been added manually. The colors seems identical to the patched code, but using the same technique the prevent the validation to mess things up is required, IMHO. - E : File `colors` loaded with the patched code, the Color Attribute node has been automatically set. Adding WIP status because this code probably needs an update of the required Blender version in `__init__.py`, but I don't know which one exactly. This patch works with 4.1.

98 KiB

150 KiB

150 KiB

181 KiB

84 KiB

👍 1

Hombre57 changed title from ~~Fix per vertex normals and colors import~~ to WIP: Fix per vertex normals and colors import

2024-07-21 17:03:41 +02:00

Cedric Steiert commented

2024-07-21 18:04:13 +02:00

Just tested in blender 4.2.
The color part works, even though the node gets weirdly placed (but i guess that cannot be influenced, maybe do some research there. Would be great if the placement could be corrected).
About the normals: With your script the file imports with flat shading (unlike stated in your description) and when set to smooth it still gets displayed weirdly (like your image B). Seems to either only work in blender 4.1 or didn't really get fixed yet. Also from the looks of it, it doesn't look like an issue with the normals, but rather marked sharp edges not being present.

C : File normals loaded with the patched code. It comes in Smooth shading by default with correct normals

Btw how do you know when an object should be shaded smooth or flat? Or is just the corrected normals part relevant?

Just tested in blender 4.2. The color part works, even though the node gets weirdly placed (but i guess that cannot be influenced, maybe do some research there. Would be great if the placement could be corrected). About the normals: With your script the file imports with flat shading (unlike stated in your description) and when set to smooth it still gets displayed weirdly (like your image B). Seems to either only work in blender 4.1 or didn't really get fixed yet. Also from the looks of it, it doesn't look like an issue with the normals, but rather marked sharp edges not being present. > C : File normals loaded with the patched code. It comes in Smooth shading by default with correct normals Btw how do you know when an object should be shaded smooth or flat? Or is just the corrected normals part relevant?

shaded_smooth_with_new_code.png

466 KiB

color_node_placement_with_new_code.png

1.2 MiB

Hombre57 commented

2024-07-22 00:36:35 +02:00

I couldn't install the extension on 4.2.0 for now, the installation fail ( I already opened an issue ). So yes, it might only work for 4.1 for now.

The color part works, even though the node gets weirdly placed

I shouldn't have delete node_vertex_color.location = (-200, 300) after line 2754. The node is correctly placed with this. I've made a patch but can push it because of a connexion error, I'll try again tomorrow.

About the normals: With your script the file imports with flat shading (unlike stated in your description) and when set to smooth it still gets displayed weirdly (like your image B).

I just discovered some... Blender weirdness, thanks to your comment. In fact, the object has to be displayed "flat" to use the normals per vertex and be displayed smooth. If one want a flat facets display, he has to delete the custom normals data. Is this behavior even documented ? "Shade smooth" does something that I don't understand, really.

Btw how do you know when an object should be shaded smooth or flat? Or is just the corrected normals part relevant?

I can't answer to that question anymore 😄

I couldn't install the extension on 4.2.0 for now, the installation fail ( [I already opened an issue](https://github.com/Tilapiatsu/blender-Universal_Multi_Importer/issues/4) ). So yes, it might only work for 4.1 for now. > The color part works, even though the node gets weirdly placed I shouldn't have delete `node_vertex_color.location = (-200, 300)` after line 2754. The node is correctly placed with this. I've made a patch but can push it because of a connexion error, I'll try again tomorrow. > About the normals: With your script the file imports with flat shading (unlike stated in your description) and when set to smooth it still gets displayed weirdly (like your image B). I just discovered some... Blender weirdness, thanks to your comment. In fact, the object has to be displayed "flat" to use the normals per vertex and be displayed smooth. If one want a flat facets display, he has to delete the custom normals data. Is this behavior even documented ? "Shade smooth" does something that I don't understand, really. > Btw how do you know when an object should be shaded smooth or flat? Or is just the corrected normals part relevant? I can't answer to that question anymore 😄

Cedric Steiert commented

2024-07-22 02:27:16 +02:00

I couldn't install the extension on 4.2.0 for now, the installation fail ( I already opened an issue ). So yes, it might only work for 4.1 for now.

I see you've created an issue for https://extensions.blender.org/add-ons/universal-multi-importer/, but here we are talking about https://extensions.blender.org/add-ons/web3d-x3d-vrml2-format/. Does you issue persist when trying to install the extension directly via preference window?

The node is correctly placed with this. I've made a patch

👍

but can push it because of a connexion error, I'll try again tomorrow.

Btw you can also use the online code editor to make quick changes in the code, without relying on any git clients.

I just discovered some... Blender weirdness, thanks to your comment. In fact, the object has to be displayed "flat" to use the normals per vertex and be displayed smooth. If one want a flat facets display, he has to delete the custom normals data. Is this behavior even documented ? "Shade smooth" does something that I don't understand, really.

Ah okay. So usage of custom normals results in a custom (smooth) shading with blenders flat shade. To sumarize your commit uses custom normals data if present in the .wrl file, whereas the default behaviour is to ignore them. Seems to work then, was just described a bit off.

Small nit picks:

you have some commented out code lines. Either remove them, or explain via code comment why they are commented out but still useful.
comments like # And delete the temporary attribute (line 2064 for example) are a bit redundant. I prefer to read the why, rather than the how (which is already there in code)
for Apply colors per vertex you have bpymesh.validate() in the else part (line 2081), but it's already done in line 2053. What's the reason behind it? Just asking for better understanding, haven't tested a change yet.

Then after your little node patch and nit pick fixes i think it's okay to get merged.

> I couldn't install the extension on 4.2.0 for now, the installation fail ( I already opened an issue ). So yes, it might only work for 4.1 for now. I see you've created an issue for https://extensions.blender.org/add-ons/universal-multi-importer/, but here we are talking about https://extensions.blender.org/add-ons/web3d-x3d-vrml2-format/. Does you issue persist when trying to install the extension directly via preference window? > The node is correctly placed with this. I've made a patch 👍 > but can push it because of a connexion error, I'll try again tomorrow. Btw you can also use the online code editor to make quick changes in the code, without relying on any git clients. > I just discovered some... Blender weirdness, thanks to your comment. In fact, the object has to be displayed "flat" to use the normals per vertex and be displayed smooth. If one want a flat facets display, he has to delete the custom normals data. Is this behavior even documented ? "Shade smooth" does something that I don't understand, really. Ah okay. So usage of custom normals results in a custom (smooth) shading with blenders flat shade. To sumarize your commit uses custom normals data if present in the .wrl file, whereas the default behaviour is to ignore them. Seems to work then, was just described a bit off. Small nit picks: - you have some commented out code lines. Either remove them, or explain via code comment why they are commented out but still useful. - comments like `# And delete the temporary attribute` (line 2064 for example) are a bit redundant. I prefer to read the why, rather than the how (which is already there in code) - for `Apply colors per vertex` you have `bpymesh.validate()` in the else part (line 2081), but it's already done in line 2053. What's the reason behind it? Just asking for better understanding, haven't tested a change yet. Then after your little node patch and nit pick fixes i think it's okay to get merged.

Hombre57 added 1 commit 2024-07-23 00:49:08 +02:00

Better node placement + code cleanup and simplification 37e6f48a33

Hombre57 commented

2024-07-23 01:48:59 +02:00

I spent way too much time on that connexion problem, I finally made the change online, as you suggested @Bujus_Krachus.

The last commit remove the useless comments and only keep one validate() call. I initially wanted to keep one without argument when neither normals nor colors per vertex were given, but I managed to wrongly indent the code, so it didn't worked as expected.

To sumarize your commit uses custom normals data if present in the .wrl file, whereas the default behaviour is to ignore them.

The default behavior was to import normals too as seen on line 1960 of the original file, but the API seems to have changed. Also, as explained in the doc, isn't Mesh.vertices supposed to be read only ?

Anyway, I looked in other import script on how to handle that and replicated there, so I hope it's fine.

I spent way too much time on that connexion problem, I finally made the change online, as you suggested @Bujus_Krachus. The last commit remove the useless comments and only keep one `validate()` call. I initially wanted to keep one without argument when neither normals nor colors per vertex were given, but I managed to wrongly indent the code, so it didn't worked as expected. > To sumarize your commit uses custom normals data if present in the .wrl file, whereas the default behaviour is to ignore them. The default behavior was to import normals too as seen on line [1960](https://projects.blender.org/extensions/io_scene_x3d/src/commit/792b2c5ce5c6159e55c5b91b1df35cdcda92fd71/source/import_x3d.py#L1960) of the original file, but the API seems to have changed. Also, as explained in the doc, isn't [Mesh.vertices](https://docs.blender.org/api/current/bpy.types.Mesh.html#bpy.types.Mesh.vertices) supposed to be read only ? Anyway, I looked in other import script on how to handle that and replicated there, so I hope it's fine.

Hombre57 changed title from ~~WIP: Fix per vertex normals and colors import~~ to Fix per vertex normals and colors import

2024-07-23 01:49:03 +02:00

Hombre57 commented

2024-07-23 01:54:08 +02:00

One more question : are normals from this file correctly imported ? Seems weird. It comes from the Siemens NX CAD software, using the same Parasolid modeler than TopSolid, so it should be as clean despite the One shape per face export. This VRML file doesn't provide normalIndex. The specs says it should use coordIndex in that case. Here the Flat attribute doesn't seem to be set by default.

One more question : are normals from this file correctly imported ? Seems weird. It comes from the Siemens NX CAD software, using the same Parasolid modeler than TopSolid, so it should be as clean despite the _One shape per face_ export. This VRML file doesn't provide `normalIndex`. The [specs](http://www.graphics.stanford.edu/courses/cs248-98-fall/Assignments/Assignment3/VRML2_Specification/spec/part1/nodesRef.html#IndexedFaceSet) says it should use `coordIndex` in that case. Here the `Flat` attribute doesn't seem to be set by default.

F.jpg

128 KiB

Machine.wrl

365 KiB

Cedric Steiert commented

2024-07-23 06:17:11 +02:00

tldr: reinclude line 1960 (as this is unrelated to your changes) and use enforce flat shading when setting custom (split) normals to fix the shading issues by including a check when setting shading for crease angle.

are normals from this file correctly imported ?

Yes, i believe so. At least the split normals.
That's why i would leave line 1960 bpymesh.vertices.foreach_set("normal", co) in. This sets the regular vertex normals, whilst your change adds the split normals for better shading control. Split Normals (or custom normals) are an extension of vertex normals and thus do different things. Would be great if you could adjust the comments to rule out any future confusion between split/regular normals.

Here the Flat attribute doesn't seem to be set by default.

That's also what i noticed with the provided file. Without your changes it looks better in smooth shaded, with your changes far better in flat shaded (due to the correct import of split normals). The only weird behaviour i see is that in the provided machine.wrl file after import it's set to smooth shaded, basically discarding the correct custom normals import. Try enforcing flat shading when custom normals per vertex get imported in the check for crease angle in line 3041 of your fork (bpydata.set_sharp_from_angle(angle=creaseAngle) should not be called for that polygon when using split normals; see below).

This VRML file doesn't provide normalIndex. The specs says it should use coordIndex in that case. Here the Flat attribute doesn't seem to be set by default.

Yes, according to the specs if normals field is not empty but the normalIndex field is, then coordIndex should be used as normalIndex. However if normals field is empty, then new normals should be generated using creaseAngle. Still, i don't get what you are trying to tell me here unfortunately...

Also, as explained in the doc, isn't Mesh.vertices supposed to be read only ?

line 1929 gives the answer, as a new mesh gets created within python. And in line 2048 the actual mesh gets updated with the python mesh. So the writing happens in a python only datablock. For convenience however BMesh should usually be used: Mesh Access.
Could be wrong here however, as i'm kinda new to blender scripting xd.
Should probably be a topic for a refactor of the addon to get rid of legacy artifacts from older api versions; but that would probably take a lot of time, as the direct bpymesh access gets used in all of the importer.

The last commit remove the useless comments and only keep one validate() call.

👍

That answer took me far too long to write lol. Hope it helps :)

tldr: reinclude line 1960 (as this is unrelated to your changes) and use enforce flat shading when setting custom (split) normals to fix the shading issues by including a check when setting shading for crease angle. > are normals from this file correctly imported ? Yes, i believe so. At least the split normals. That's why i would leave line 1960 `bpymesh.vertices.foreach_set("normal", co)` in. This sets the regular vertex normals, whilst your change adds the split normals for better shading control. Split Normals (or custom normals) are an extension of vertex normals and thus do different things. Would be great if you could adjust the comments to rule out any future confusion between split/regular normals. > Here the Flat attribute doesn't seem to be set by default. That's also what i noticed with the provided file. Without your changes it looks better in smooth shaded, with your changes far better in flat shaded (due to the correct import of split normals). The only weird behaviour i see is that in the provided `machine.wrl` file after import it's set to smooth shaded, basically discarding the correct custom normals import. Try enforcing flat shading when custom normals per vertex get imported in the check for crease angle in [line 3041 of your fork](https://projects.blender.org/Hombre57/io_scene_x3d/src/commit/37e6f48a33fb959782d31dbfa9b2c6ae59889990/source/import_x3d.py#L3041) (`bpydata.set_sharp_from_angle(angle=creaseAngle)` should not be called for that polygon when using split normals; see below). > This VRML file doesn't provide normalIndex. The specs says it should use coordIndex in that case. Here the Flat attribute doesn't seem to be set by default. Yes, according to the specs if normals field is not empty but the normalIndex field is, then coordIndex should be used as normalIndex. However if normals field is empty, then new normals should be generated using creaseAngle. Still, i don't get what you are trying to tell me here unfortunately... > Also, as explained in the doc, isn't Mesh.vertices supposed to be read only ? [line 1929](https://projects.blender.org/extensions/io_scene_x3d/src/commit/792b2c5ce5c6159e55c5b91b1df35cdcda92fd71/source/import_x3d.py#L1929) gives the answer, as a new mesh gets created within python. And in [line 2048](https://projects.blender.org/extensions/io_scene_x3d/src/commit/792b2c5ce5c6159e55c5b91b1df35cdcda92fd71/source/import_x3d.py#L2048) the actual mesh gets updated with the python mesh. So the writing happens in a python only datablock. For convenience however BMesh should usually be used: [Mesh Access](https://projects.blender.org/extensions/io_scene_x3d/src/commit/792b2c5ce5c6159e55c5b91b1df35cdcda92fd71/source/import_x3d.py#L2048). Could be wrong here however, as i'm kinda new to blender scripting xd. Should probably be a topic for a refactor of the addon to get rid of legacy artifacts from older api versions; but that would probably take a lot of time, as the direct `bpymesh` access gets used in all of the importer. > The last commit remove the useless comments and only keep one validate() call. 👍 That answer took me far too long to write lol. Hope it helps :)

Cedric Steiert commented

2024-07-23 06:18:32 +02:00

fyi according to my current latent knowledge you haven't done a fix, but added two features 🥇

Cedric Steiert requested changes 2024-07-23 06:22:03 +02:00

Dismissed

Cedric Steiert left a comment

see comment above

Hombre57 added 1 commit 2024-08-03 23:04:07 +02:00

Add support for per vertex normals w/o normalIndex 790902323c

Also suppresse a useless 'validate' (forgotten from the previous commit)
and better named color attribute

Hombre57 commented

2024-08-03 23:41:21 +02:00

@Bujus_Krachus I finally had time to work on this. I've added the support of per vertex normals w/o normal index and changed the conditions for setting the polygon smooth.

Depending on the data in the WRL file, it'll either use normals_split_custom_set (i.e. per coin) or bpymesh.polygons.foreach_set("normal", co) (i.e. per face), though I've not tested this one.

On the colore side, bpymesh.vertex_colors.new().data.foreach_set('color', cco) should be replaced since vertex_colors i deprecated. I can look out later on how to replace it.

As for the Machine.wrl file, I've investigated why the rendering is so bad. I find out that the lack of precision for the normal is triggering a recompute of the normals, not just their normalisation. You can clearly see that with Plan2.wrl file where the 3 digits normal values has a length of 0.999924 (see image G for the normals). Edit the file and switch to the second normal which has a length of 1.000000, and it will be used as is (see image H). I'll open another ticket for that, as I don't think CAD software will enhance their WRL files for more precision.

That answer took me far too long to write lol. Hope it helps :)

Yes it helped a lot and was worth it 🙏.

@Bujus_Krachus I finally had time to work on this. I've added the support of per vertex normals w/o normal index and changed the conditions for setting the polygon smooth. Depending on the data in the WRL file, it'll either use `normals_split_custom_set` (i.e. per coin) or `bpymesh.polygons.foreach_set("normal", co)` (i.e. per face), though I've not tested this one. On the colore side, `bpymesh.vertex_colors.new().data.foreach_set('color', cco)` should be replaced since `vertex_colors` i deprecated. I can look out later on how to replace it. As for the **Machine.wrl** file, I've investigated why the rendering is so bad. I find out that the lack of precision for the normal is triggering a recompute of the normals, not just their normalisation. You can clearly see that with **Plan2.wrl** file where the 3 digits normal values has a length of 0.999924 (see image G for the normals). Edit the file and switch to the second normal which has a length of 1.000000, and it will be used as is (see image H). I'll open another ticket for that, as I don't think CAD software will enhance their WRL files for more precision. > That answer took me far too long to write lol. Hope it helps :) Yes it helped a lot and was worth it 🙏.

H.png

62 KiB

Plan2.wrl

1.6 KiB

G.png

66 KiB

Hombre57 commented

2024-08-04 00:25:32 +02:00

PS : As for reintroducing line 1960, I've tested that and didn't see any difference. Are you sure it's required since normals_split_custom_set does all the job ? Also, shouldn't we use bpymesh.polygon_normals instead of bpymesh.vertice here ?

PS : As for reintroducing line 1960, I've tested that and didn't see any difference. Are you sure it's required since `normals_split_custom_set` does all the job ? Also, shouldn't we use `bpymesh.polygon_normals` instead of `bpymesh.vertice` here ?

Cedric Steiert requested changes 2024-08-04 01:27:23 +02:00

Dismissed

Cedric Steiert left a comment

I finally had time to work on this

Great to hear. Haven't tested yet with other files than your provided ones, but changes seem mostly valid.

However please revert the changes from 790902323c for the color part, as now loading your provided colors.wrl file will throw KeyError: 'bpy_prop_collection[key]: key "Col" not found'. Renaming node_vertex_color.layer_name = "ColorPerCoin" back to node_vertex_color.layer_name = "Col" works, but the material will be black, thus revert all color related changes from that commit or fix it.

The custom normals part now works for provided file, due to the added check.

PS : As for reintroducing line 1960, I've tested that and didn't see any difference. Are you sure it's required since normals_split_custom_set does all the job ?

Sure? No, but i believe so, however i'm no expert here. In previous (visual) testing i've found no difference with/without though...

Also, shouldn't we use bpymesh.polygon_normals instead of bpymesh.vertice here ?

bpymesh.vertices should probably be correct, as we set the vertex normals. Each face/polygon has multiple vertex normals but only one poly normal.

If it works and the old code isn't interfering with the changes i would leave it in if no other reported issues or tasks refer to that code piece, until definetly sure. Otherwise i'd suggest creating a cleanup PR, which removes redundant or legacy code (like that it will be easier to track where stuff breaks if it breaks).

Will approve the PR after the color fix; the bpymesh.vertices.foreach_set("normal", co) will be your decision to make, my suggestion is to leave it in.

> I finally had time to work on this Great to hear. Haven't tested yet with other files than your provided ones, but changes seem mostly valid. However please revert the changes from https://projects.blender.org/extensions/io_scene_x3d/commit/790902323cf0835fe15d0e89e674be98cf056a4b for the color part, as now loading your provided `colors.wrl` file will throw `KeyError: 'bpy_prop_collection[key]: key "Col" not found'`. Renaming `node_vertex_color.layer_name = "ColorPerCoin"` back to `node_vertex_color.layer_name = "Col"` works, but the material will be black, thus revert all color related changes from that commit or fix it. The custom normals part now works for provided file, due to the added check. > PS : As for reintroducing line 1960, I've tested that and didn't see any difference. Are you sure it's required since normals_split_custom_set does all the job ? Sure? No, but i believe so, however i'm no expert here. In previous (visual) testing i've found no difference with/without though... > Also, shouldn't we use bpymesh.polygon_normals instead of bpymesh.vertice here ? `bpymesh.vertices` should probably be correct, as we set the vertex normals. Each face/polygon has multiple vertex normals but only one poly normal. If it works and the old code isn't interfering with the changes i would leave it in if no other reported issues or tasks refer to that code piece, until definetly sure. Otherwise i'd suggest creating a cleanup PR, which removes redundant or legacy code (like that it will be easier to track where stuff breaks if it breaks). Will approve the PR after the color fix; the `bpymesh.vertices.foreach_set("normal", co)` will be your decision to make, my suggestion is to leave it in.

Hombre57 added 1 commit 2024-08-04 11:38:27 +02:00

Fix the color attribute's name to avoid script crash 30c46be9ff

Also reintroduce `bpymesh.vertices.foreach_set` for Per Vertx normals as
requested

Hombre57 added 1 commit 2024-08-04 12:42:55 +02:00

Merge branch 'main' into per-vertex_normals_and_colors 6007d1651d

Hombre57 commented

2024-08-04 12:44:24 +02:00

I think it's useless, but I don't know the API enough to decide, so I'll take your conservative advice and leave it in.

The color attribute is fixed, my fault of a half imported patch, sorry.

I think it's ready to merge.

I think it's useless, but I don't know the API enough to decide, so I'll take your conservative advice and leave it in. The color attribute is fixed, my fault of a half imported patch, sorry. I think it's ready to merge.

👍 1

Cedric Steiert approved these changes 2024-08-04 13:45:32 +02:00

Dismissed

Cedric Steiert left a comment

Thanks for the changes, works now and should be ready to merge :)

Hombre57 added 1 commit 2024-08-04 17:14:26 +02:00

Add vector normalization step to handle vectors with too few decimals 0b42c37959

Hombre57 commented

2024-08-04 17:19:49 +02:00

@Bujus_Krachus The fix for lack of decimals in vector components is pretty simple. I've committed a patch to add the normalization step. The Machine.wrl file is correctly imported now, with no penalty in processing time and precision for other files already correct. Tell me if this should be reverted and part of another PR. However it's totally related to normal import, though not only per vertex ones.

@Bujus_Krachus The fix for lack of decimals in vector components is pretty simple. I've committed a patch to add the normalization step. The `Machine.wrl` file is correctly imported now, with no penalty in processing time and precision for other files already correct. Tell me if this should be reverted and part of another PR. However it's totally related to normal import, though not only per vertex ones.

Cedric Steiert requested changes 2024-08-04 18:25:49 +02:00

Dismissed

Cedric Steiert left a comment

Well, thanks for the addition. It can stay as now the imported mesh looks even cleaner than ever and though is totally realted to the PR with the suggested demo files.

However you're missing import mathutils at the start of the file, probably forgot to include that in the commit.

Well, thanks for the addition. It can stay as now the imported mesh looks even cleaner than ever and though is totally realted to the PR with the suggested demo files. However you're missing `import mathutils` at the start of the file, probably forgot to include that in the commit.

Hombre57 added 1 commit 2024-08-04 19:13:04 +02:00

Add forgotten mathutils import d17730d608

Hombre57 commented

2024-08-04 19:13:28 +02:00

Damn ! I'm not used to produce such crappy commits 😶‍. Back and forth testing and yep, that line was gone. It still works w/o that line here, so that's why I didn't noticed it was gone.

Cedric Steiert approved these changes 2024-08-04 19:20:49 +02:00

Dismissed

Cedric Steiert left a comment

Damn ! I'm not used to produce such crappy commits 😶‍. Back and forth testing and yep, that line was gone. It still works w/o that line here, so that's why I didn't noticed it was gone.

No biggie, happens to all of us ;)

> Damn ! I'm not used to produce such crappy commits 😶‍. Back and forth testing and yep, that line was gone. It still works w/o that line here, so that's why I didn't noticed it was gone. No biggie, happens to all of us ;)

Cedric Steiert commented

2024-08-04 19:24:01 +02:00

Btw as you seem quite knowledgeble with the wrl format feel free to suggest further bug fixes or code improvements if you want and have some spare time on your hand; contributions are always welcome

Cedric Steiert referenced this pull request

2024-08-04 23:30:50 +02:00

fix list index out of range for color_per_vertex #2

Hombre57 added 1 commit 2024-08-05 13:59:05 +02:00

Use plain english for Color attribute's name e8940cbc8c

Hombre57 referenced this pull request

2024-08-05 17:49:56 +02:00

fix list index out of range for color_per_vertex #2

Cedric Steiert referenced this pull request

2024-08-05 18:06:48 +02:00

fix list index out of range for color_per_vertex #2

Cedric Steiert referenced this pull request

2024-08-05 18:08:05 +02:00

fix list index out of range for color_per_vertex #2

Hombre57 referenced this pull request

2024-08-05 20:44:31 +02:00

X3D/VRML import: Fix blender/blender-addons#98442, check if USE-based cache is set in IndexedFaceSet #7

Cedric Steiert referenced this issue from a commit

2024-08-06 17:19:40 +02:00

fix list index out of range for color_per_vertex (#2)

Cedric Steiert commented

2024-08-06 18:01:49 +02:00

Hi, i merged my PRs into main, then attempted yours and saw a quite noticeable difference in shading of the file mentioned in #2. See the screenshots. Would be great if you could take a look at it.

current master without your PR, Color Attribute node added to a few sample objects:

current master with your PR:

notice how the color is far lighter, is that correct how it is?

Also we now have two identical color attributes (maybe reduce to one again, or what's the difference? Most likely caused since 790902323c):

Hi, i merged my PRs into main, then attempted yours and saw a quite noticeable difference in shading of the file mentioned in https://projects.blender.org/extensions/io_scene_x3d/pulls/2. See the screenshots. Would be great if you could take a look at it. current master without your PR, Color Attribute node added to a few sample objects: ![grafik](/attachments/4aaa04e1-199e-47fd-97a5-2fa0409fe121) current master with your PR: ![grafik](/attachments/30177e81-9f05-499b-a518-f78cf56692d9) notice how the color is far lighter, is that correct how it is? Also we now have two identical color attributes (maybe reduce to one again, or what's the difference? Most likely caused since https://projects.blender.org/extensions/io_scene_x3d/commit/790902323cf0835fe15d0e89e674be98cf056a4b): ![grafik](/attachments/00780b5c-5ea8-4ddf-94c6-efa754dc632f)

grafik.png

757 KiB

grafik.png

1.1 MiB

grafik.png

37 KiB

Hombre57 added 1 commit 2024-08-06 22:15:37 +02:00

Merge branch 'main' into per-vertex_normals_and_colors cf37b9d1b7

Hombre57 commented

2024-08-07 00:58:43 +02:00

@Bujus_Krachus It looks like that the 2 colors are defined with the sames values but with different result.

when set with bpymesh.vertex_colors (a deprecated function), it sets a default color attribute name Col showing a darker red.
The second image in the originale post of blender/blender-addons#101604 also shows this dark red color, as well as some other VRLM online viewer.
when set with the new color_attributes mechanism, it sets my newly created ColorPerCorner attribute showing a lighter red. The color values out of the VRML files are 0.84 0.28 0.28. Setting this value as base color to one shape has identical result than custom vertex colors, so I guess that's the right color.

When looking at Blender's code, vertex_colors is in linear color space. So the new mechanism behave differently and doesn't convert the color automatically, or I didn't find a setting somewhere to do so. That being said, I'm not sure of the color space used in VRML files. It says in §4.3.3 :

Colors are specified in the RGB (Red-Green-Blue) color space and are restricted to the 0.0 to 1.0 range.

So are we supposed to convert all colors to a special color space ?

@Bujus_Krachus It looks like that the 2 colors are defined with the sames values but with different result. - when set with `bpymesh.vertex_colors` ([a deprecated function](https://docs.blender.org/api/current/bpy.types.Mesh.html#bpy.types.Mesh.vertex_colors)), it sets a default color attribute name `Col` showing a darker red. The second image in the originale post of blender/blender-addons#101604 also shows this dark red color, as well as some other VRLM online viewer. - when set with the new [color_attributes](https://docs.blender.org/api/current/bpy.types.Mesh.html#bpy.types.Mesh.color_attributes) mechanism, it sets my newly created `ColorPerCorner` attribute showing a lighter red. The color values out of the VRML files are `0.84 0.28 0.28`. Setting this value as base color to one shape has identical result than custom vertex colors, so I guess that's the right color. ![I.png](/attachments/474b1eb0-09a6-4028-a9d6-ee43e71b51ff) When looking at Blender's code, `vertex_colors` is in linear color space. So the new mechanism behave differently and doesn't convert the color automatically, or I didn't find a setting somewhere to do so. That being said, I'm not sure of the color space used in VRML files. It says in §4.3.3 : > Colors are specified in the RGB (Red-Green-Blue) color space and are restricted to the 0.0 to 1.0 range. So are we supposed to convert all colors to a special color space ?

I.png

165 KiB

Cedric Steiert commented

2024-08-07 02:55:32 +02:00

you are totally right. Using the new API vs legacy API produces different results. This happens due to color space mismatch as seen here: 765b1c6b53 and like you explained above. Thanks for investiagting that so quickly :)

In the importer the vertex color Col gets saved as linear using the lagcy API, whereas ColorPerCorner gets saved in srgb by the newer API. For now i'd account for that and follow the old scheme, meaning introducing a color space conversion (because other importers/software seem to use linear as well and to not introduce breaking changes to the addon):

    def linear_to_srgb(linear):
        if linear <= 0.0031308:
            return linear * 12.92
        else:
            return 1.055 * (linear ** (1.0 / 2.4)) - 0.055

    def srgb_to_linear(srgb_value):
        if srgb_value <= 0.04045:
            return srgb_value / 12.92
        else:
            return ((srgb_value + 0.055) / 1.055) ** 2.4

    # Apply colors per vertex
    if colors and color_per_vertex:
        cco2 = [0.0 for x in range(int(len(bpymesh.attributes["temp_custom_colors"].data)*4))]
        bpymesh.attributes["temp_custom_colors"].data.foreach_get("color", cco2)
        # convert color spaces to account for api changes
        cco2 = [srgb_to_linear(col_val) for col_val in cco2]
        bpymesh.color_attributes.new('ColorPerCorner', 'FLOAT_COLOR', 'CORNER')
        bpymesh.color_attributes["ColorPerCorner"].data.foreach_set("color", cco2)
        bpymesh.attributes.remove(bpymesh.attributes["temp_custom_colors"])

As a future improvement (as i've read you're interested in doing so), maybe let the users choose on the import which color space to display in.

Also to remove redundancy for users i'd propose this:

        if color_per_vertex:
            # Mesh must be validated before assigning colors, but validation might
            # reorder corners. We must store colors in a temporary attribute
            bpymesh.attributes.new("temp_custom_colors", 'FLOAT_COLOR', 'CORNER')
            bpymesh.attributes["temp_custom_colors"].data.foreach_set("color", cco)
        else:
            d = bpymesh.vertex_colors.new().data
            d.foreach_set('color', cco)

note: for consistency it may be good to update that to the newer api as well (including conversion)

future note: there are 2 other occurencies of vertex_colors.new() which could be updated -> not related to the current PRs topic

you are totally right. Using the new API vs legacy API produces different results. This happens due to color space mismatch as seen here: https://projects.blender.org/blender/blender/commit/765b1c6b53da655d55af18f883d91fd1238fb773 and like you explained above. Thanks for investiagting that so quickly :) In the importer the vertex color `Col` gets saved as linear using the lagcy API, whereas `ColorPerCorner` gets saved in srgb by the newer API. For now i'd account for that and follow the old scheme, meaning introducing a color space conversion (because other importers/software seem to use linear as well and to not introduce breaking changes to the addon): ``` def linear_to_srgb(linear): if linear <= 0.0031308: return linear * 12.92 else: return 1.055 * (linear ** (1.0 / 2.4)) - 0.055 def srgb_to_linear(srgb_value): if srgb_value <= 0.04045: return srgb_value / 12.92 else: return ((srgb_value + 0.055) / 1.055) ** 2.4 # Apply colors per vertex if colors and color_per_vertex: cco2 = [0.0 for x in range(int(len(bpymesh.attributes["temp_custom_colors"].data)*4))] bpymesh.attributes["temp_custom_colors"].data.foreach_get("color", cco2) # convert color spaces to account for api changes cco2 = [srgb_to_linear(col_val) for col_val in cco2] bpymesh.color_attributes.new('ColorPerCorner', 'FLOAT_COLOR', 'CORNER') bpymesh.color_attributes["ColorPerCorner"].data.foreach_set("color", cco2) bpymesh.attributes.remove(bpymesh.attributes["temp_custom_colors"]) ``` _As a future improvement (as i've read you're interested in doing so), maybe let the users choose on the import which color space to display in._ Also to remove redundancy for users i'd propose this: ``` if color_per_vertex: # Mesh must be validated before assigning colors, but validation might # reorder corners. We must store colors in a temporary attribute bpymesh.attributes.new("temp_custom_colors", 'FLOAT_COLOR', 'CORNER') bpymesh.attributes["temp_custom_colors"].data.foreach_set("color", cco) else: d = bpymesh.vertex_colors.new().data d.foreach_set('color', cco) ``` _note: for consistency it may be good to update that to the newer api as well (including conversion)_ _future note: there are 2 other occurencies of `vertex_colors.new()` which could be updated -> not related to the current PRs topic_

Hombre57 added 1 commit 2024-08-07 11:33:31 +02:00

Handle color space for vertex colors using the color_attributes API c957f6bc57

Thanks to @Bujus_Krachus for the patch.

Cedric Steiert approved these changes 2024-08-07 11:40:32 +02:00

Cedric Steiert left a comment

Alright, then ready to merge. Any pending changes?

Hombre57 commented

2024-08-07 11:41:16 +02:00

Done ! Thanks for the patch @Bujus_Krachus 👍

As a future improvement (as i've read you're interested in doing so), maybe let the users choose on the import which color space to display in.

Yes, I'll probably add this options later, after my summer vacations 😉

Ready to merge for me.

Done ! Thanks for the patch @Bujus_Krachus 👍 > As a future improvement (as i've read you're interested in doing so), maybe let the users choose on the import which color space to display in. Yes, I'll probably add this options later, after my summer vacations :wink: Ready to merge for me.

Cedric Steiert merged commit 975d4ebb2e into main

2024-08-07 12:04:13 +02:00

Cedric Steiert referenced this issue from a commit

2024-08-07 12:04:14 +02:00

Fix per vertex normals and colors import (#6)

Cedric Steiert commented

2024-08-07 12:04:36 +02:00

Thanks for the contribution, merged.

Hombre57 deleted branch per-vertex_normals_and_colors

2024-08-09 12:35:21 +02:00

Cedric Steiert referenced this pull request

2024-08-09 19:10:31 +02:00