fix list index out of range for color_per_vertex #2

Cedric Steiert · 2024-07-19T22:18:16+02:00

Cedric Steiert commented

2024-07-19 22:18:16 +02:00

fixes blender/blender-addons#101604 by using the index position instead of the actual vertex value to access the rgb list, because in the provided file the required filed colorIndex was missing (zero length).
Now the file from mentioned issue can get imported, however in rendered view the colors are still missing. But that should probably be another issue, as it seems unrelated (this is caused by the missing ColorAttribute node in the material setup and gets fixed/added in #6).

Now the code is able to:

rebuild colorIndex field if -1 face end markers are not matching with coordIndex (detected by comparing count of end markers in both arrays)
use regular index position if no colorIndex is given (no rebuild is needed here, just different access)
handling vertex colors for both cases (with/without colorIndex)

fixes https://projects.blender.org/blender/blender-addons/issues/101604 by using the index position instead of the actual vertex value to access the rgb list, because in the provided file the required filed colorIndex was missing (zero length). Now the file from mentioned issue can get imported, however in rendered view the colors are still missing. But that should probably be another issue, as it seems unrelated (this is caused by the missing ColorAttribute node in the material setup and gets fixed/added in https://projects.blender.org/extensions/io_scene_x3d/pulls/6). Now the code is able to: - rebuild colorIndex field if -1 face end markers are not matching with coordIndex (detected by comparing count of end markers in both arrays) - use regular index position if no colorIndex is given (no rebuild is needed here, just different access) - handling vertex colors for both cases (with/without colorIndex)

Cedric Steiert added 1 commit 2024-07-19 22:18:17 +02:00

fix list index out of range for color_per_vertex 9b74290f68

fixes blender/blender-addons#101604 by using the index position instead of the actual vertex value to access the rgb list.
Now the file from mentioned issue can get imported, however in rendered view the colors are still missing. But that should probably be another issue, as it seems unrelated.

Hombre57 commented

2024-07-20 14:56:12 +02:00

Have you tested your patch with a correctly formed VRML file, i.e. with face vertex color index ending with '-1' like other per vertex nodes does ?

http://graphcomp.com/info/specs/sgi/vrml/spec/part1/nodesRef.html#IndexedFaceSet

The colorIndex field must contain at least as many indices as the coordIndex field, and must contain end-of-face markers (-1) in exactly the same places as the coordIndex field.

Have you tested your patch with a correctly formed VRML file, i.e. with face vertex color index ending with '-1' like other per vertex nodes does ? http://graphcomp.com/info/specs/sgi/vrml/spec/part1/nodesRef.html#IndexedFaceSet > The colorIndex field must contain at least as many indices as the coordIndex field, and must contain end-of-face markers (-1) in exactly the same places as the coordIndex field.

Cedric Steiert commented

2024-07-20 15:08:35 +02:00

Hi @Hombre57 could you be so kind and provide a "valid" file to test, which would apply in that case? Currently don't have the time to look one up/create one.

Have you tested your patch with a correctly formed VRML file, i.e. with face vertex color index ending with '-1' like other per vertex nodes does ?

In my local copy it works with several files, however i'm not too experienced with the mentioned tech standard, so those files may not even trigger that case. But i don't see why they wouldn't work now, i just fixed a tiny logic error in the code. Anyways i'd be glad to test with your files.

Hi @Hombre57 could you be so kind and provide a "valid" file to test, which would apply in that case? Currently don't have the time to look one up/create one. > Have you tested your patch with a correctly formed VRML file, i.e. with face vertex color index ending with '-1' like other per vertex nodes does ? In my local copy it works with several files, however i'm not too experienced with the mentioned tech standard, so those files may not even trigger that case. But i don't see why they wouldn't work now, i just fixed a tiny logic error in the code. Anyways i'd be glad to test with your files.

Hombre57 commented

2024-07-20 15:46:16 +02:00

@Bujus_Krachus Here are some files. The screenshots are taken after importing the "corrected" version.

9b74290f68.jpg

111 KiB

9b74290f68 - expected.jpg

89 KiB

1002609-corrected.wrl

107 KiB

1002609-broken.wrl

103 KiB

Cedric Steiert commented

2024-07-20 15:59:30 +02:00

@Hombre57 thank you for taking the time. Indeed, my proposed "fix" is not a fix at all as it seems.
Do you have an idea/quick fix on how to handle such "broken" files? Because programs like FreeWRL are able to display them all correctly...

@Hombre57 thank you for taking the time. Indeed, my proposed "fix" is not a fix at all as it seems. Do you have an idea/quick fix on how to handle such "broken" files? Because programs like FreeWRL are able to display them all correctly...

Cedric Steiert changed title from ~~fix list index out of range for color_per_vertex~~ to WIP: fix list index out of range for color_per_vertex

2024-07-20 15:59:50 +02:00

Hombre57 commented

2024-07-20 16:31:49 +02:00

@Bujus_Krachus

In short :

// Test will be fast for correct files, more time consuming for broken files
if ('-1' not in color_index)
   // Build a temporary list and place '-1' at the same positions than in 'coordIndex'

@Bujus_Krachus In short : ``` // Test will be fast for correct files, more time consuming for broken files if ('-1' not in color_index) // Build a temporary list and place '-1' at the same positions than in 'coordIndex' ```

👍 1

Cedric Steiert added 1 commit 2024-07-20 17:02:21 +02:00

fix detection of working wrl files 6cb7049fa3

Usually .wrl files with vertex colors have and colorIndex Field with -1 values as face end markers. 
In the original issue file no colorIndex Field was present, thus we check for 0 length, in which case a regular index count gets used. Now both issue file and correct file should import and display correctly.
However to further bulletproof things, we should also check for invalid colorIndexes (without correct number of -1s), this still has to be done.

Cedric Steiert commented

2024-07-20 17:04:47 +02:00

@Hombre57 thanks for the suggestion, will add that soon. Currently i've added a simple length check because the broken file did not have an colorIndex field (in such cases we don't have to rebuild the colorIndex field in my opinion, which saves a bit of performance).

👍 1

Cedric Steiert added 1 commit 2024-07-20 17:28:39 +02:00

add colorIndex valid check and rebuild option for face end markers e8e3c6454c

Now the code is able to:
- rebuild colorIndex field if -1 face end markers are not matching with coordIndex
- use regular index position if no colorIndex is given
- handling vertex colors for both cases (with/without colorIndex)

Cedric Steiert changed title from ~~WIP: fix list index out of range for color_per_vertex~~ to fix list index out of range for color_per_vertex

2024-07-20 17:29:05 +02:00

Cedric Steiert commented

2024-07-20 17:29:51 +02:00

@Hombre57 i've included your change requests. Now all cases should work fine.

Hombre57 commented

2024-07-21 12:32:51 +02:00

@Bujus_Krachus That's clean ! 😃 Thanks for the update.

Who's reviewing and accepting the PR ? (yes, I'm new here)

@Bujus_Krachus That's clean ! 😃 Thanks for the update. Who's reviewing and accepting the PR ? (yes, I'm new here)

Cedric Steiert commented

2024-07-21 13:13:57 +02:00

Who's reviewing and accepting the PR ? (yes, I'm new here)

Tbh idk. Since the addon moved to the extensions platform it doesn't have a maintainer anymore and the official blender team isn't really responsible either.

This add-on is offered as it is and maintaned by the community, no support expected.

> Who's reviewing and accepting the PR ? (yes, I'm new here) Tbh idk. Since the addon moved to the extensions platform it doesn't have a maintainer anymore and the official blender team isn't really responsible either. > This add-on is offered as it is and maintaned by the community, no support expected.

Hombre57 commented

2024-08-04 20:32:06 +02:00

@Bujus_Krachus

Don't you mean index instead of color_index here ?

@Bujus_Krachus Don't you mean `index` instead of `color_index` [here](https://projects.blender.org/Bujus_Krachus/io_scene_x3d/src/commit/e8e3c6454c02defbddb8a8eff99bf4f6636c2b16/source/import_x3d.py#L1997) ?

Cedric Steiert added 1 commit 2024-08-04 23:19:49 +02:00

simplify code for case Color per vertex without index 0f47714f59

as processPerVertexIndex(color_index) will default to returning faces for that case (as the color_index is empty), we can save the function call and provide the faces directly.

Cedric Steiert commented

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

Hi @Hombre57, you are partially correct.

Comparison
However, when using coordIndex (or index) it will have Ambient Occlusion Pre-baked into the material as it seems. I don’t think this is intended behavior for the importer (yet).

Compared to using color_index:

Btw the missing colorAttribute node and smooth shading will probably be fixed by a merge with your PR (#6).
Compared to freeWRL:

Explanation:
processPerVertexIndex(color_index) will default to returning faces (basically coordIndex, just more abstracted into faces already) if the parameter ind is empty (like it’s the case here for mentioned case) unlike when using index, then it (coordIndex) will get unfolded; so in the new commit I will simplify the code, to save some few function calls to hopefully improve performance for that case a tiny bit.

If you are other opinion or have some tech knowledge i don't have, please elaborate and ideally attach a file where the current solution breaks things. Thank you for bringing it up.

We could however leverage the observed behavior of pre-baked AO (if that’s even correct and not just a coincidence for the file of the original issue) by creating a new option in the importer; still the implementation for the other cases and material types is missing, so that would have to be a todo.

Hi @Hombre57, you are partially correct. **Comparison** However, when using `coordIndex` (or `index`) it will have Ambient Occlusion Pre-baked into the material as it seems. I don’t think this is intended behavior for the importer (yet). <img src="/attachments/edba0fa6-9ff7-4870-afb7-08b5cb602eab" width="300"> Compared to using `color_index`: <img src="/attachments/a45193cf-0a71-4a67-b817-c5f22775185c" width="300"> _Btw the missing colorAttribute node and smooth shading will probably be fixed by a merge with your PR (https://projects.blender.org/extensions/io_scene_x3d/pulls/6)._ Compared to freeWRL: <img src="/attachments/fa6565d9-2d2f-4f0a-9508-65cab6923a9d" width="300"> **Explanation:** `processPerVertexIndex(color_index)` will default to returning `faces` (basically coordIndex, just more abstracted into faces already) if the parameter `ind` is empty (like it’s the case here for mentioned case) unlike when using `index`, then it (`coordIndex`) will get unfolded; so in the new commit I will simplify the code, to save some few function calls to hopefully improve performance for that case a tiny bit. If you are other opinion or have some tech knowledge i don't have, please elaborate and ideally attach a file where the current solution breaks things. Thank you for bringing it up. We could however leverage the observed behavior of pre-baked AO (if that’s even correct and not just a coincidence for the file of the original issue) by creating a new option in the importer; still the implementation for the other cases and material types is missing, so that would have to be a todo.

grafik.png

406 KiB

grafik.png

441 KiB

grafik.png

64 KiB

Hombre57 commented

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

@Bujus_Krachus

Btw the missing colorAttribute node and smooth shading will probably be fixed by a merge with your PR

Yes, I manually merged this branch in PR #6 and it loaded with normals and colors. Here is what I get.

Thank you for bringing it up.

No problem ! I was just surprised to see normal_index here and overlooked that special case in processPerVertexIndex 😅. Great if you can optimize your code even more.

Now we just need a maintainer to merge your PR first, I'll resolve conflicts there after.

@Bujus_Krachus > Btw the missing colorAttribute node and smooth shading will probably be fixed by a merge with your PR Yes, I manually merged this branch in PR #6 and it loaded with normals and colors. Here is what I get. ![TAD_10_-_435_100_TWISTER.jpg](/attachments/bf0e0b6f-9e27-422c-9a32-b32221bb98b4) > Thank you for bringing it up. No problem ! I was just surprised to see `normal_index` here and overlooked that special case in `processPerVertexIndex` 😅. Great if you can optimize your code even more. Now we just need a maintainer to merge your PR first, I'll resolve conflicts there after.

TAD_10_-_435_100_TWISTER.jpg

224 KiB

Cedric Steiert commented

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

@Hombre57

Yes, I manually merged this branch in PR #6 and it loaded with normals and colors. Here is what I get.

Great, looks exactly how it should (with smooth shading and colors applied).

No problem ! I was just surprised to see normal_index here and overlooked that special case in processPerVertexIndex

Yeah, haha. You indeed let me question what i was thinking back then, took a while to find that part again lol

Now we just need a maintainer to merge your PR first, I'll resolve conflicts there after.

In #7 Nika (volunteer as admin for the extension platform) asked, if i wanted to become the future maintainer, unfortunately since then no more activity occured. If you want, you can comment and tag her as well, if you're interested. Else either her oder someone from the blender team is responsible (another PR got merged by Campbell Barton, but no activity since then either). I'll wait another week for a response and will then request some reviews in hope someone will respond.

@Hombre57 > Yes, I manually merged this branch in PR #6 and it loaded with normals and colors. Here is what I get. Great, looks exactly how it should (with smooth shading and colors applied). > No problem ! I was just surprised to see normal_index here and overlooked that special case in processPerVertexIndex Yeah, haha. You indeed let me question what i was thinking back then, took a while to find that part again lol > Now we just need a maintainer to merge your PR first, I'll resolve conflicts there after. In https://projects.blender.org/extensions/io_scene_x3d/pulls/7 Nika (volunteer as admin for the extension platform) asked, if i wanted to become the future maintainer, unfortunately since then no more activity occured. If you want, you can comment and tag her as well, if you're interested. Else either her oder someone from the blender team is responsible (another PR got merged by Campbell Barton, but no activity since then either). I'll wait another week for a response and will then request some reviews in hope someone will respond.

Cedric Steiert added 1 commit 2024-08-06 17:16:23 +02:00

Merge branch 'main' into fix-list-index-out-of-range-vertex-color b566bb6c87

Cedric Steiert merged commit c86b76ee55 into main

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

Cedric Steiert deleted branch fix-list-index-out-of-range-vertex-color

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

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 referenced this pull request

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

Fix per vertex normals and colors import #6