search

CesiumGS / obj2gltf

Convert OBJ assets to glTF
Apache License 2.0
1.71k stars 307 forks source link

Fix objs with interleaved materials #155

Closed lilleyse closed 6 years ago

lilleyse commented 6 years ago

Fixes a bug introduced in https://github.com/AnalyticalGraphicsInc/obj2gltf/pull/153 where the vertex count was not getting reset properly when an obj has interleaved materials.

lilleyse commented 6 years ago

@OmarShehata could you quickly look at this and https://github.com/AnalyticalGraphicsInc/obj2gltf/pull/156

OmarShehata commented 6 years ago

@lilleyse this might be a fault of the model I tested it with, but I'm curious what your test was for this fix.

I tested it with the csxirc5h.i2b.obj model in the glTF viewer (https://gltf-viewer.donmccurdy.com/). In master it reports these errors:

Code Message Pointer
ACCESSOR_NON_UNIT 4 accessor elements not of unit length: 0. [AGGREGATED] /accessors/172
ACCESSOR_NON_UNIT 2 accessor elements not of unit length: 0. [AGGREGATED] /accessors/1588

And in this branch it reports:

Code Message Pointer
ACCESSOR_INDEX_OOB Indices accessor element at index 91 has vertex index 137 that exceeds number of available vertices 113. /accessors/499
ACCESSOR_INDEX_OOB Indices accessor element at index 92 has vertex index 136 that exceeds number of available vertices 113. /accessors/499
ACCESSOR_INDEX_OOB Indices accessor element at index 94 has vertex index 134 that exceeds number of available vertices 113. /accessors/499
ACCESSOR_INDEX_OOB Indices accessor element at index 95 has vertex index 133 that exceeds number of available vertices 113. /accessors/499
ACCESSOR_INDEX_OOB Indices accessor element at index 97 has vertex index 133 that exceeds number of available vertices 113. /accessors/499
ACCESSOR_INDEX_OOB Indices accessor element at index 100 has vertex index 130 that exceeds number of available vertices 113. /accessors/499
ACCESSOR_INDEX_OOB Indices accessor element at index 101 has vertex index 129 that exceeds number of available vertices 113. /accessors/499
ACCESSOR_INDEX_OOB Indices accessor element at index 103 has vertex index 127 that exceeds number of available vertices 113. /accessors/499
ACCESSOR_INDEX_OOB Indices accessor element at index 104 has vertex index 126 that exceeds number of available vertices 113. /accessors/499
ACCESSOR_INDEX_OOB Indices accessor element at index 106 has vertex index 126 that exceeds number of available vertices 113. /accessors/499
ACCESSOR_INDEX_OOB Indices accessor element at index 109 has vertex index 123 that exceeds number of available vertices 113. /accessors/499
ACCESSOR_INDEX_OOB Indices accessor element at index 110 has vertex index 122 that exceeds number of available vertices 113. /accessors/499
ACCESSOR_INDEX_OOB Indices accessor element at index 112 has vertex index 119 that exceeds number of available vertices 113. /accessors/499
ACCESSOR_INDEX_OOB Indices accessor element at index 113 has vertex index 118 that exceeds number of available vertices 113. /accessors/499
OmarShehata commented 6 years ago

The code and the unit test look good though.

lilleyse commented 6 years ago

Thanks for catching that @OmarShehata. It should be fixed now.

It also seems like the model is not getting triangulated correctly in some areas. I'll open a separate PR for that. Hopefully https://github.com/AnalyticalGraphicsInc/obj2gltf/issues/147 fixes it.

OmarShehata commented 6 years ago

No errors now, so I think this should be good to merge.

lilleyse commented 6 years ago

Cool, thanks @OmarShehata.