Three.js: ColladaLoader: Support multiple sets of uv coordinates

[MohmadAyman]

Description of the problem

Hi,

I've some problems with the texture mapping to dae object, after examining ColladaLoader I've found that in the old version

    case 'offsetV':
    case 'repeatU':
    case 'repeatV':

        this.texOpts[ child.nodeName ] = parseFloat( child.textContent );

        break;

while in the new one

        case 'repeatV':
        case 'offsetU':
        case 'offsetV':

            data.technique[ child.nodeName ] = parseFloat( child.textContent );
            break;

So after editing the new ColldaLoader like this the mapping worked like expected

        case 'repeatV':

            data.technique[ child.nodeName ] = parseFloat( child.textContent );
            break;

Old ColladaLoader screenshot

New ColladaLoader screenshot

mesh file

Three.js version

• [x] Dev
• [ ] r87
• [ ] ...

Browser

• [x] All of them
• [ ] Chrome
• [ ] Firefox
• [ ] Internet Explorer

OS

• [x] All of them
• [ ] Windows
• [ ] macOS
• [ ] Linux
• [ ] Android
• [ ] iOS

Hardware Requirements (graphics card, VR Device, ...)

[Mugen87]

I'm not sure i get it 😅 . The current implementation parses texture parameters like this:

https://github.com/mrdoob/three.js/blob/21ab28ec24611f303d0f5975f8d3236a8f863dde/examples/js/loaders/ColladaLoader.js#L1287-L1292

What kind of change do you propose? Besides, i don't think that a change in the parsing section solves the problem. Wr probably need to change this section, where the parameters are applied to the actual texture object...

https://github.com/mrdoob/three.js/blob/21ab28ec24611f303d0f5975f8d3236a8f863dde/examples/js/loaders/ColladaLoader.js#L1399-L1416

[MohmadAyman]

mb for not being clear :sweat_smile:
I propose

 case 'repeatU': 
 case 'repeatV':
    data.technique[ child.nodeName ] = parseFloat( child.textContent ); 
    break;  
 case 'offsetU': 
 case 'offsetV':

like the old Loader used to do.

Yeah exchanging the offset and repeat in the second section would also solve it. I'm not really sure which is the best place to fix this.

Thanks!

[Mugen87]

Maybe you make a PR so i can better see and test your changes. Deal? 😊

[MohmadAyman]

Here you go :relieved:

[MohmadAyman]

@Mugen87 here is the whole model zipped, textures and the mesh included.

Here is the whole model zipped.

Thanks.

[Mugen87]

Thanks, i'll have a look at it.

[Mugen87]

I've tested your dae file but i'm afraid there are not textures included. So let me ask, how do you load the textures and apply them to your mesh?

I expected a structure like in this file. The textures are defined in the tag library_images.

<library_images>
  <image id="Stormtrooper_D" name="Stormtrooper_D">
    <init_from>Stormtrooper_D.jpg</init_from>
  </image>
</library_images>

[MohmadAyman]

Thanks for looking into it.

Sorry forgot to mention that I didn't use to use library_images for that.

I load the textures like this:
First load the mesh

var dae = loader.loadCollada(); 

then

var texture = textureLoader.load(textureUri);
dae.material.map = texture;

[Mugen87]

Assuming your model has correct uv data, you have to manually set the appropriate texture parameter. For example:

var texture = textureLoader.load( textureUri );
texture.wrapS = texture.wrapT = THREE.RepeatWrapping;
dae.material.map = texture;

I don't understand why this workflow worked with the old loader. If no textures are defined in the dae file, the implementation is unable to create any kinds of texture objects.

[MohmadAyman]

Okay I see, but why wouldn't it be a problem with the loader well since it only appeared when it was upgraded?

Well the loading code was written quite a time ago, we used to only set the texture.wrapS and texture.wrapT if the material had a scaling factor.

var texture = this.textureLoader.load(material.texture);
        if (material.scale)
        {
          texture.wrapS = texture.wrapT = THREE.RepeatWrapping;

I've tried your approach for loading the texture, but unfortunately nth changed.

[Mugen87]

Ok, i'll revisit this at the weekend.

[Mugen87]

BTW: I don't think that the usage material.scale makes any sense in this context. LineDashedMaterial is the only material that has a scale property.

[Mugen87]

Ahhh, i think i found the issue. It is actually a problem in the loader! Your file contains the following uv information:

<input semantic="TEXCOORD" source="#geom-Box005-map1" offset="2" set="0"/>
<input semantic="TEXCOORD" source="#geom-Box005-map2" offset="3" set="1"/>

As you can see, two sets of uv coordinates. The new collada always picks the last set of uvs, which is a different behavior compared to the old loader. It seems to be necessary to evaluate the set attribute in some way in order to get correct results.

[MohmadAyman]

Oh I see, so thats why.

I'll try to have a look at it this weekend to try to solve it.

[Mugen87]

Cool! You might want to start at parseGeometryPrimitive():
https://github.com/mrdoob/three.js/blob/a57b090b5c6a7307ccabd94965e56f5568594166/examples/js/loaders/ColladaLoader.js#L1881-L1887

After we have read the set attribute for an input tag, we need to find a way to store multiple uv sets for the semantic TEXCOORD. Right now, an existing entry just gets overwritten. This new behavior will cause some refactoring within this function and at all places where these values are processed e.g.
https://github.com/mrdoob/three.js/blob/c1afee8fea2157a3846b7b9bdf00d4f69d7076eb/examples/js/loaders/ColladaLoader.js#L2040-L2043

The old loader implementation might give you some hints how to correctly use these multiple uv sets.

[mrdoob]

@MohmadAyman Any luck?

[MohmadAyman]

Sorry have been very busy with sth else lately, I'll try to give it a run this weekend, Thanks for the reminder :+1:

[MohmadAyman]

Sorry, I couldn't find time to tackle the issue this week, so I guess it is considered open now for anyone to work on it.